[PATCH 1/2] [media] cec: Move capability check inside #if

[PATCH 1/2] [media] cec: Move capability check inside #if

[Lee Jones]

If CONFIG_RC_CORE is not enabled then none of the RC code will be
executed anyway, so we're placing the capability check inside the

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/media/cec/cec-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/cec/cec-core.c b/drivers/media/cec/cec-core.c
index 37217e2..06a312c 100644
--- a/drivers/media/cec/cec-core.c
+++ b/drivers/media/cec/cec-core.c
@@ -234,10 +234,10 @@ struct cec_adapter *cec_allocate_adapter(const struct cec_adap_ops *ops,
 		return ERR_PTR(res);
 	}
 
+#if IS_REACHABLE(CONFIG_RC_CORE)
 	if (!(caps & CEC_CAP_RC))
 		return adap;
 
-#if IS_REACHABLE(CONFIG_RC_CORE)
 	/* Prepare the RC input device */
 	adap->rc = rc_allocate_device(RC_DRIVER_SCANCODE);
 	if (!adap->rc) {
-- 
2.9.3

[PATCH 1/2] [media] cec: Move capability check inside #if

[Lee Jones]

If CONFIG_RC_CORE is not enabled then none of the RC code will be
executed anyway, so we're placing the capability check inside the

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/media/cec/cec-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/cec/cec-core.c b/drivers/media/cec/cec-core.c
index 37217e2..06a312c 100644
--- a/drivers/media/cec/cec-core.c
+++ b/drivers/media/cec/cec-core.c
@@ -234,10 +234,10 @@ struct cec_adapter *cec_allocate_adapter(const struct cec_adap_ops *ops,
 		return ERR_PTR(res);
 	}
 
+#if IS_REACHABLE(CONFIG_RC_CORE)
 	if (!(caps & CEC_CAP_RC))
 		return adap;
 
-#if IS_REACHABLE(CONFIG_RC_CORE)
 	/* Prepare the RC input device */
 	adap->rc = rc_allocate_device(RC_DRIVER_SCANCODE);
 	if (!adap->rc) {
-- 
2.9.3

[PATCH 2/2] [media] cec: Fix runtime BUG when (CONFIG_RC_CORE && !CEC_CAP_RC)

[Lee Jones]

Currently when the RC Core is enabled (reachable) core code located
in cec_register_adapter() attempts to populate the RC structure with
a pointer to the 'parent' passed in by the caller.

Unfortunately if the caller did not specify RC capibility when calling
cec_allocate_adapter(), then there will be no RC structure to populate.

This causes a "NULL pointer dereference" error.

Fixes: f51e80804f0 ("[media] cec: pass parent device in register(), not allocate()")
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/media/cec/cec-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/cec/cec-core.c b/drivers/media/cec/cec-core.c
index 06a312c..d64937b 100644
--- a/drivers/media/cec/cec-core.c
+++ b/drivers/media/cec/cec-core.c
@@ -286,8 +286,8 @@ int cec_register_adapter(struct cec_adapter *adap,
 	adap->devnode.dev.parent = parent;
 
 #if IS_REACHABLE(CONFIG_RC_CORE)
-	adap->rc->dev.parent = parent;
 	if (adap->capabilities & CEC_CAP_RC) {
+		adap->rc->dev.parent = parent;
 		res = rc_register_device(adap->rc);
 
 		if (res) {
-- 
2.9.3

[PATCH 2/2] [media] cec: Fix runtime BUG when (CONFIG_RC_CORE && !CEC_CAP_RC)

[Lee Jones]

Currently when the RC Core is enabled (reachable) core code located
in cec_register_adapter() attempts to populate the RC structure with
a pointer to the 'parent' passed in by the caller.

Unfortunately if the caller did not specify RC capibility when calling
cec_allocate_adapter(), then there will be no RC structure to populate.

This causes a "NULL pointer dereference" error.

Fixes: f51e80804f0 ("[media] cec: pass parent device in register(), not allocate()")
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/media/cec/cec-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/cec/cec-core.c b/drivers/media/cec/cec-core.c
index 06a312c..d64937b 100644
--- a/drivers/media/cec/cec-core.c
+++ b/drivers/media/cec/cec-core.c
@@ -286,8 +286,8 @@ int cec_register_adapter(struct cec_adapter *adap,
 	adap->devnode.dev.parent = parent;
 
 #if IS_REACHABLE(CONFIG_RC_CORE)
-	adap->rc->dev.parent = parent;
 	if (adap->capabilities & CEC_CAP_RC) {
+		adap->rc->dev.parent = parent;
 		res = rc_register_device(adap->rc);
 
 		if (res) {
-- 
2.9.3

Re: [PATCH 1/2] [media] cec: Move capability check inside #if

[Hans Verkuil]

On 04/04/2017 02:32 PM, Lee Jones wrote:
> If CONFIG_RC_CORE is not enabled then none of the RC code will be
> executed anyway, so we're placing the capability check inside the
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/media/cec/cec-core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/cec/cec-core.c b/drivers/media/cec/cec-core.c
> index 37217e2..06a312c 100644
> --- a/drivers/media/cec/cec-core.c
> +++ b/drivers/media/cec/cec-core.c
> @@ -234,10 +234,10 @@ struct cec_adapter *cec_allocate_adapter(const struct cec_adap_ops *ops,
>  		return ERR_PTR(res);
>  	}
>  
> +#if IS_REACHABLE(CONFIG_RC_CORE)
>  	if (!(caps & CEC_CAP_RC))
>  		return adap;
>  
> -#if IS_REACHABLE(CONFIG_RC_CORE)
>  	/* Prepare the RC input device */
>  	adap->rc = rc_allocate_device(RC_DRIVER_SCANCODE);
>  	if (!adap->rc) {
> 

Not true, there is an #else further down.

That said, this code is clearly a bit confusing.

It would be better if at the beginning of the function we'd have this:

#if !IS_REACHABLE(CONFIG_RC_CORE)
	caps &= ~CEC_CAP_RC;
#endif

and then drop the #else bit and (as you do in this patch) move the #if up.

Can you make a new patch for this?

Thanks!

	Hans

[PATCH 1/2] [media] cec: Move capability check inside #if

[Hans Verkuil]

On 04/04/2017 02:32 PM, Lee Jones wrote:
> If CONFIG_RC_CORE is not enabled then none of the RC code will be
> executed anyway, so we're placing the capability check inside the
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/media/cec/cec-core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/cec/cec-core.c b/drivers/media/cec/cec-core.c
> index 37217e2..06a312c 100644
> --- a/drivers/media/cec/cec-core.c
> +++ b/drivers/media/cec/cec-core.c
> @@ -234,10 +234,10 @@ struct cec_adapter *cec_allocate_adapter(const struct cec_adap_ops *ops,
>  		return ERR_PTR(res);
>  	}
>  
> +#if IS_REACHABLE(CONFIG_RC_CORE)
>  	if (!(caps & CEC_CAP_RC))
>  		return adap;
>  
> -#if IS_REACHABLE(CONFIG_RC_CORE)
>  	/* Prepare the RC input device */
>  	adap->rc = rc_allocate_device(RC_DRIVER_SCANCODE);
>  	if (!adap->rc) {
> 

Not true, there is an #else further down.

That said, this code is clearly a bit confusing.

It would be better if at the beginning of the function we'd have this:

#if !IS_REACHABLE(CONFIG_RC_CORE)
	caps &= ~CEC_CAP_RC;
#endif

and then drop the #else bit and (as you do in this patch) move the #if up.

Can you make a new patch for this?

Thanks!

	Hans

Re: [PATCH 2/2] [media] cec: Fix runtime BUG when (CONFIG_RC_CORE && !CEC_CAP_RC)

[Hans Verkuil]

On 04/04/2017 02:32 PM, Lee Jones wrote:
> Currently when the RC Core is enabled (reachable) core code located
> in cec_register_adapter() attempts to populate the RC structure with
> a pointer to the 'parent' passed in by the caller.
> 
> Unfortunately if the caller did not specify RC capibility when calling
> cec_allocate_adapter(), then there will be no RC structure to populate.
> 
> This causes a "NULL pointer dereference" error.
> 
> Fixes: f51e80804f0 ("[media] cec: pass parent device in register(), not allocate()")
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

Oops! Thanks for the report. I'll take this for 4.12.

Regards,

	Hans

> ---
>  drivers/media/cec/cec-core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/cec/cec-core.c b/drivers/media/cec/cec-core.c
> index 06a312c..d64937b 100644
> --- a/drivers/media/cec/cec-core.c
> +++ b/drivers/media/cec/cec-core.c
> @@ -286,8 +286,8 @@ int cec_register_adapter(struct cec_adapter *adap,
>  	adap->devnode.dev.parent = parent;
>  
>  #if IS_REACHABLE(CONFIG_RC_CORE)
> -	adap->rc->dev.parent = parent;
>  	if (adap->capabilities & CEC_CAP_RC) {
> +		adap->rc->dev.parent = parent;
>  		res = rc_register_device(adap->rc);
>  
>  		if (res) {
> 

[PATCH 2/2] [media] cec: Fix runtime BUG when (CONFIG_RC_CORE && !CEC_CAP_RC)

[Hans Verkuil]

On 04/04/2017 02:32 PM, Lee Jones wrote:
> Currently when the RC Core is enabled (reachable) core code located
> in cec_register_adapter() attempts to populate the RC structure with
> a pointer to the 'parent' passed in by the caller.
> 
> Unfortunately if the caller did not specify RC capibility when calling
> cec_allocate_adapter(), then there will be no RC structure to populate.
> 
> This causes a "NULL pointer dereference" error.
> 
> Fixes: f51e80804f0 ("[media] cec: pass parent device in register(), not allocate()")
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

Oops! Thanks for the report. I'll take this for 4.12.

Regards,

	Hans

> ---
>  drivers/media/cec/cec-core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/cec/cec-core.c b/drivers/media/cec/cec-core.c
> index 06a312c..d64937b 100644
> --- a/drivers/media/cec/cec-core.c
> +++ b/drivers/media/cec/cec-core.c
> @@ -286,8 +286,8 @@ int cec_register_adapter(struct cec_adapter *adap,
>  	adap->devnode.dev.parent = parent;
>  
>  #if IS_REACHABLE(CONFIG_RC_CORE)
> -	adap->rc->dev.parent = parent;
>  	if (adap->capabilities & CEC_CAP_RC) {
> +		adap->rc->dev.parent = parent;
>  		res = rc_register_device(adap->rc);
>  
>  		if (res) {
> 

Re: [PATCH 1/2] [media] cec: Move capability check inside #if

[Lee Jones]

On Tue, 04 Apr 2017, Hans Verkuil wrote:

> On 04/04/2017 02:32 PM, Lee Jones wrote:
> > If CONFIG_RC_CORE is not enabled then none of the RC code will be
> > executed anyway, so we're placing the capability check inside the
> > 
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  drivers/media/cec/cec-core.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/cec/cec-core.c b/drivers/media/cec/cec-core.c
> > index 37217e2..06a312c 100644
> > --- a/drivers/media/cec/cec-core.c
> > +++ b/drivers/media/cec/cec-core.c
> > @@ -234,10 +234,10 @@ struct cec_adapter *cec_allocate_adapter(const struct cec_adap_ops *ops,
> >  		return ERR_PTR(res);
> >  	}
> >  
> > +#if IS_REACHABLE(CONFIG_RC_CORE)
> >  	if (!(caps & CEC_CAP_RC))
> >  		return adap;
> >  
> > -#if IS_REACHABLE(CONFIG_RC_CORE)
> >  	/* Prepare the RC input device */
> >  	adap->rc = rc_allocate_device(RC_DRIVER_SCANCODE);
> >  	if (!adap->rc) {
> > 
> 
> Not true, there is an #else further down.

I saw the #else.  It's inert code that becomes function-less.

> That said, this code is clearly a bit confusing.
> 
> It would be better if at the beginning of the function we'd have this:
> 
> #if !IS_REACHABLE(CONFIG_RC_CORE)
> 	caps &= ~CEC_CAP_RC;
> #endif
> 
> and then drop the #else bit and (as you do in this patch) move the #if up.
> 
> Can you make a new patch for this?

Sure.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 1/2] [media] cec: Move capability check inside #if

[Lee Jones]

On Tue, 04 Apr 2017, Hans Verkuil wrote:

> On 04/04/2017 02:32 PM, Lee Jones wrote:
> > If CONFIG_RC_CORE is not enabled then none of the RC code will be
> > executed anyway, so we're placing the capability check inside the
> > 
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  drivers/media/cec/cec-core.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/cec/cec-core.c b/drivers/media/cec/cec-core.c
> > index 37217e2..06a312c 100644
> > --- a/drivers/media/cec/cec-core.c
> > +++ b/drivers/media/cec/cec-core.c
> > @@ -234,10 +234,10 @@ struct cec_adapter *cec_allocate_adapter(const struct cec_adap_ops *ops,
> >  		return ERR_PTR(res);
> >  	}
> >  
> > +#if IS_REACHABLE(CONFIG_RC_CORE)
> >  	if (!(caps & CEC_CAP_RC))
> >  		return adap;
> >  
> > -#if IS_REACHABLE(CONFIG_RC_CORE)
> >  	/* Prepare the RC input device */
> >  	adap->rc = rc_allocate_device(RC_DRIVER_SCANCODE);
> >  	if (!adap->rc) {
> > 
> 
> Not true, there is an #else further down.

I saw the #else.  It's inert code that becomes function-less.

> That said, this code is clearly a bit confusing.
> 
> It would be better if at the beginning of the function we'd have this:
> 
> #if !IS_REACHABLE(CONFIG_RC_CORE)
> 	caps &= ~CEC_CAP_RC;
> #endif
> 
> and then drop the #else bit and (as you do in this patch) move the #if up.
> 
> Can you make a new patch for this?

Sure.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 2/2] [media] cec: Fix runtime BUG when (CONFIG_RC_CORE && !CEC_CAP_RC)

[Lee Jones]

On Tue, 04 Apr 2017, Hans Verkuil wrote:

> On 04/04/2017 02:32 PM, Lee Jones wrote:
> > Currently when the RC Core is enabled (reachable) core code located
> > in cec_register_adapter() attempts to populate the RC structure with
> > a pointer to the 'parent' passed in by the caller.
> > 
> > Unfortunately if the caller did not specify RC capibility when calling
> > cec_allocate_adapter(), then there will be no RC structure to populate.
> > 
> > This causes a "NULL pointer dereference" error.
> > 
> > Fixes: f51e80804f0 ("[media] cec: pass parent device in register(), not allocate()")
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> 
> Oops! Thanks for the report. I'll take this for 4.12.

Since this is a -fix, it should really go in for v4.11.

> > ---
> >  drivers/media/cec/cec-core.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/cec/cec-core.c b/drivers/media/cec/cec-core.c
> > index 06a312c..d64937b 100644
> > --- a/drivers/media/cec/cec-core.c
> > +++ b/drivers/media/cec/cec-core.c
> > @@ -286,8 +286,8 @@ int cec_register_adapter(struct cec_adapter *adap,
> >  	adap->devnode.dev.parent = parent;
> >  
> >  #if IS_REACHABLE(CONFIG_RC_CORE)
> > -	adap->rc->dev.parent = parent;
> >  	if (adap->capabilities & CEC_CAP_RC) {
> > +		adap->rc->dev.parent = parent;
> >  		res = rc_register_device(adap->rc);
> >  
> >  		if (res) {
> > 
> 

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 2/2] [media] cec: Fix runtime BUG when (CONFIG_RC_CORE && !CEC_CAP_RC)

[Lee Jones]

On Tue, 04 Apr 2017, Hans Verkuil wrote:

> On 04/04/2017 02:32 PM, Lee Jones wrote:
> > Currently when the RC Core is enabled (reachable) core code located
> > in cec_register_adapter() attempts to populate the RC structure with
> > a pointer to the 'parent' passed in by the caller.
> > 
> > Unfortunately if the caller did not specify RC capibility when calling
> > cec_allocate_adapter(), then there will be no RC structure to populate.
> > 
> > This causes a "NULL pointer dereference" error.
> > 
> > Fixes: f51e80804f0 ("[media] cec: pass parent device in register(), not allocate()")
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> 
> Oops! Thanks for the report. I'll take this for 4.12.

Since this is a -fix, it should really go in for v4.11.

> > ---
> >  drivers/media/cec/cec-core.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/cec/cec-core.c b/drivers/media/cec/cec-core.c
> > index 06a312c..d64937b 100644
> > --- a/drivers/media/cec/cec-core.c
> > +++ b/drivers/media/cec/cec-core.c
> > @@ -286,8 +286,8 @@ int cec_register_adapter(struct cec_adapter *adap,
> >  	adap->devnode.dev.parent = parent;
> >  
> >  #if IS_REACHABLE(CONFIG_RC_CORE)
> > -	adap->rc->dev.parent = parent;
> >  	if (adap->capabilities & CEC_CAP_RC) {
> > +		adap->rc->dev.parent = parent;
> >  		res = rc_register_device(adap->rc);
> >  
> >  		if (res) {
> > 
> 

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/2] [media] cec: Move capability check inside #if

[Hans Verkuil]

On 04/04/2017 02:54 PM, Lee Jones wrote:
> On Tue, 04 Apr 2017, Hans Verkuil wrote:
> 
>> On 04/04/2017 02:32 PM, Lee Jones wrote:
>>> If CONFIG_RC_CORE is not enabled then none of the RC code will be
>>> executed anyway, so we're placing the capability check inside the
>>>
>>> Signed-off-by: Lee Jones <lee.jones@linaro.org>
>>> ---
>>>  drivers/media/cec/cec-core.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/media/cec/cec-core.c b/drivers/media/cec/cec-core.c
>>> index 37217e2..06a312c 100644
>>> --- a/drivers/media/cec/cec-core.c
>>> +++ b/drivers/media/cec/cec-core.c
>>> @@ -234,10 +234,10 @@ struct cec_adapter *cec_allocate_adapter(const struct cec_adap_ops *ops,
>>>  		return ERR_PTR(res);
>>>  	}
>>>  
>>> +#if IS_REACHABLE(CONFIG_RC_CORE)
>>>  	if (!(caps & CEC_CAP_RC))
>>>  		return adap;
>>>  
>>> -#if IS_REACHABLE(CONFIG_RC_CORE)
>>>  	/* Prepare the RC input device */
>>>  	adap->rc = rc_allocate_device(RC_DRIVER_SCANCODE);
>>>  	if (!adap->rc) {
>>>
>>
>> Not true, there is an #else further down.
> 
> I saw the #else.  It's inert code that becomes function-less.

No, it isn't. It clears the CAP_RC bit so it isn't returned in the CEC_ADAP_G_CAPS ioctl.
Drivers set this cap bit if they want RC support (they typically want it), but if the
config option isn't there then the capability should be removed.

Regards,

	Hans

> 
>> That said, this code is clearly a bit confusing.
>>
>> It would be better if at the beginning of the function we'd have this:
>>
>> #if !IS_REACHABLE(CONFIG_RC_CORE)
>> 	caps &= ~CEC_CAP_RC;
>> #endif
>>
>> and then drop the #else bit and (as you do in this patch) move the #if up.
>>
>> Can you make a new patch for this?
> 
> Sure.
> 

[PATCH 1/2] [media] cec: Move capability check inside #if

[Hans Verkuil]

On 04/04/2017 02:54 PM, Lee Jones wrote:
> On Tue, 04 Apr 2017, Hans Verkuil wrote:
> 
>> On 04/04/2017 02:32 PM, Lee Jones wrote:
>>> If CONFIG_RC_CORE is not enabled then none of the RC code will be
>>> executed anyway, so we're placing the capability check inside the
>>>
>>> Signed-off-by: Lee Jones <lee.jones@linaro.org>
>>> ---
>>>  drivers/media/cec/cec-core.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/media/cec/cec-core.c b/drivers/media/cec/cec-core.c
>>> index 37217e2..06a312c 100644
>>> --- a/drivers/media/cec/cec-core.c
>>> +++ b/drivers/media/cec/cec-core.c
>>> @@ -234,10 +234,10 @@ struct cec_adapter *cec_allocate_adapter(const struct cec_adap_ops *ops,
>>>  		return ERR_PTR(res);
>>>  	}
>>>  
>>> +#if IS_REACHABLE(CONFIG_RC_CORE)
>>>  	if (!(caps & CEC_CAP_RC))
>>>  		return adap;
>>>  
>>> -#if IS_REACHABLE(CONFIG_RC_CORE)
>>>  	/* Prepare the RC input device */
>>>  	adap->rc = rc_allocate_device(RC_DRIVER_SCANCODE);
>>>  	if (!adap->rc) {
>>>
>>
>> Not true, there is an #else further down.
> 
> I saw the #else.  It's inert code that becomes function-less.

No, it isn't. It clears the CAP_RC bit so it isn't returned in the CEC_ADAP_G_CAPS ioctl.
Drivers set this cap bit if they want RC support (they typically want it), but if the
config option isn't there then the capability should be removed.

Regards,

	Hans

> 
>> That said, this code is clearly a bit confusing.
>>
>> It would be better if at the beginning of the function we'd have this:
>>
>> #if !IS_REACHABLE(CONFIG_RC_CORE)
>> 	caps &= ~CEC_CAP_RC;
>> #endif
>>
>> and then drop the #else bit and (as you do in this patch) move the #if up.
>>
>> Can you make a new patch for this?
> 
> Sure.
> 

Re: [PATCH 1/2] [media] cec: Move capability check inside #if

[Lee Jones]

On Tue, 04 Apr 2017, Lee Jones wrote:

> On Tue, 04 Apr 2017, Hans Verkuil wrote:
> 
> > On 04/04/2017 02:32 PM, Lee Jones wrote:
> > > If CONFIG_RC_CORE is not enabled then none of the RC code will be
> > > executed anyway, so we're placing the capability check inside the
> > > 
> > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > ---
> > >  drivers/media/cec/cec-core.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/media/cec/cec-core.c b/drivers/media/cec/cec-core.c
> > > index 37217e2..06a312c 100644
> > > --- a/drivers/media/cec/cec-core.c
> > > +++ b/drivers/media/cec/cec-core.c
> > > @@ -234,10 +234,10 @@ struct cec_adapter *cec_allocate_adapter(const struct cec_adap_ops *ops,
> > >  		return ERR_PTR(res);
> > >  	}
> > >  
> > > +#if IS_REACHABLE(CONFIG_RC_CORE)
> > >  	if (!(caps & CEC_CAP_RC))
> > >  		return adap;
> > >  
> > > -#if IS_REACHABLE(CONFIG_RC_CORE)
> > >  	/* Prepare the RC input device */
> > >  	adap->rc = rc_allocate_device(RC_DRIVER_SCANCODE);
> > >  	if (!adap->rc) {
> > > 
> > 
> > Not true, there is an #else further down.
> 
> I saw the #else.  It's inert code that becomes function-less.
> 
> > That said, this code is clearly a bit confusing.
> > 
> > It would be better if at the beginning of the function we'd have this:
> > 
> > #if !IS_REACHABLE(CONFIG_RC_CORE)
> > 	caps &= ~CEC_CAP_RC;
> > #endif
> > 
> > and then drop the #else bit and (as you do in this patch) move the #if up.
> > 
> > Can you make a new patch for this?
> 
> Sure.

No wait, sorry!  This patch is the correct fix.

'caps' is already indicating !CEC_CAP_RC, which is right.

What we're trying to do here is only consider looking at the
capabilities if the RC Core is enabled.  If it is not enabled, the #if
still does the right thing and makes sure that the caps are updated.

Please take another look at the semantics.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 1/2] [media] cec: Move capability check inside #if

[Lee Jones]

On Tue, 04 Apr 2017, Lee Jones wrote:

> On Tue, 04 Apr 2017, Hans Verkuil wrote:
> 
> > On 04/04/2017 02:32 PM, Lee Jones wrote:
> > > If CONFIG_RC_CORE is not enabled then none of the RC code will be
> > > executed anyway, so we're placing the capability check inside the
> > > 
> > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > ---
> > >  drivers/media/cec/cec-core.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/media/cec/cec-core.c b/drivers/media/cec/cec-core.c
> > > index 37217e2..06a312c 100644
> > > --- a/drivers/media/cec/cec-core.c
> > > +++ b/drivers/media/cec/cec-core.c
> > > @@ -234,10 +234,10 @@ struct cec_adapter *cec_allocate_adapter(const struct cec_adap_ops *ops,
> > >  		return ERR_PTR(res);
> > >  	}
> > >  
> > > +#if IS_REACHABLE(CONFIG_RC_CORE)
> > >  	if (!(caps & CEC_CAP_RC))
> > >  		return adap;
> > >  
> > > -#if IS_REACHABLE(CONFIG_RC_CORE)
> > >  	/* Prepare the RC input device */
> > >  	adap->rc = rc_allocate_device(RC_DRIVER_SCANCODE);
> > >  	if (!adap->rc) {
> > > 
> > 
> > Not true, there is an #else further down.
> 
> I saw the #else.  It's inert code that becomes function-less.
> 
> > That said, this code is clearly a bit confusing.
> > 
> > It would be better if at the beginning of the function we'd have this:
> > 
> > #if !IS_REACHABLE(CONFIG_RC_CORE)
> > 	caps &= ~CEC_CAP_RC;
> > #endif
> > 
> > and then drop the #else bit and (as you do in this patch) move the #if up.
> > 
> > Can you make a new patch for this?
> 
> Sure.

No wait, sorry!  This patch is the correct fix.

'caps' is already indicating !CEC_CAP_RC, which is right.

What we're trying to do here is only consider looking at the
capabilities if the RC Core is enabled.  If it is not enabled, the #if
still does the right thing and makes sure that the caps are updated.

Please take another look at the semantics.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/2] [media] cec: Move capability check inside #if

[Hans Verkuil]

On 04/04/2017 03:01 PM, Lee Jones wrote:
> On Tue, 04 Apr 2017, Lee Jones wrote:
> 
>> On Tue, 04 Apr 2017, Hans Verkuil wrote:
>>
>>> On 04/04/2017 02:32 PM, Lee Jones wrote:
>>>> If CONFIG_RC_CORE is not enabled then none of the RC code will be
>>>> executed anyway, so we're placing the capability check inside the
>>>>
>>>> Signed-off-by: Lee Jones <lee.jones@linaro.org>
>>>> ---
>>>>  drivers/media/cec/cec-core.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/media/cec/cec-core.c b/drivers/media/cec/cec-core.c
>>>> index 37217e2..06a312c 100644
>>>> --- a/drivers/media/cec/cec-core.c
>>>> +++ b/drivers/media/cec/cec-core.c
>>>> @@ -234,10 +234,10 @@ struct cec_adapter *cec_allocate_adapter(const struct cec_adap_ops *ops,
>>>>  		return ERR_PTR(res);
>>>>  	}
>>>>  
>>>> +#if IS_REACHABLE(CONFIG_RC_CORE)
>>>>  	if (!(caps & CEC_CAP_RC))
>>>>  		return adap;
>>>>  
>>>> -#if IS_REACHABLE(CONFIG_RC_CORE)
>>>>  	/* Prepare the RC input device */
>>>>  	adap->rc = rc_allocate_device(RC_DRIVER_SCANCODE);
>>>>  	if (!adap->rc) {
>>>>
>>>
>>> Not true, there is an #else further down.
>>
>> I saw the #else.  It's inert code that becomes function-less.
>>
>>> That said, this code is clearly a bit confusing.
>>>
>>> It would be better if at the beginning of the function we'd have this:
>>>
>>> #if !IS_REACHABLE(CONFIG_RC_CORE)
>>> 	caps &= ~CEC_CAP_RC;
>>> #endif
>>>
>>> and then drop the #else bit and (as you do in this patch) move the #if up.
>>>
>>> Can you make a new patch for this?
>>
>> Sure.
> 
> No wait, sorry!  This patch is the correct fix.
> 
> 'caps' is already indicating !CEC_CAP_RC, which is right.
> 
> What we're trying to do here is only consider looking at the
> capabilities if the RC Core is enabled.  If it is not enabled, the #if
> still does the right thing and makes sure that the caps are updated.
> 
> Please take another look at the semantics.

Ah, yes. You are right. But so am I: the code is just unnecessarily confusing
as is seen by this discussion.

I still would like to see a patch with my proposed solution. The control flow
is much easier to understand that way.

Regards,

	Hans

[PATCH 1/2] [media] cec: Move capability check inside #if

[Hans Verkuil]

On 04/04/2017 03:01 PM, Lee Jones wrote:
> On Tue, 04 Apr 2017, Lee Jones wrote:
> 
>> On Tue, 04 Apr 2017, Hans Verkuil wrote:
>>
>>> On 04/04/2017 02:32 PM, Lee Jones wrote:
>>>> If CONFIG_RC_CORE is not enabled then none of the RC code will be
>>>> executed anyway, so we're placing the capability check inside the
>>>>
>>>> Signed-off-by: Lee Jones <lee.jones@linaro.org>
>>>> ---
>>>>  drivers/media/cec/cec-core.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/media/cec/cec-core.c b/drivers/media/cec/cec-core.c
>>>> index 37217e2..06a312c 100644
>>>> --- a/drivers/media/cec/cec-core.c
>>>> +++ b/drivers/media/cec/cec-core.c
>>>> @@ -234,10 +234,10 @@ struct cec_adapter *cec_allocate_adapter(const struct cec_adap_ops *ops,
>>>>  		return ERR_PTR(res);
>>>>  	}
>>>>  
>>>> +#if IS_REACHABLE(CONFIG_RC_CORE)
>>>>  	if (!(caps & CEC_CAP_RC))
>>>>  		return adap;
>>>>  
>>>> -#if IS_REACHABLE(CONFIG_RC_CORE)
>>>>  	/* Prepare the RC input device */
>>>>  	adap->rc = rc_allocate_device(RC_DRIVER_SCANCODE);
>>>>  	if (!adap->rc) {
>>>>
>>>
>>> Not true, there is an #else further down.
>>
>> I saw the #else.  It's inert code that becomes function-less.
>>
>>> That said, this code is clearly a bit confusing.
>>>
>>> It would be better if at the beginning of the function we'd have this:
>>>
>>> #if !IS_REACHABLE(CONFIG_RC_CORE)
>>> 	caps &= ~CEC_CAP_RC;
>>> #endif
>>>
>>> and then drop the #else bit and (as you do in this patch) move the #if up.
>>>
>>> Can you make a new patch for this?
>>
>> Sure.
> 
> No wait, sorry!  This patch is the correct fix.
> 
> 'caps' is already indicating !CEC_CAP_RC, which is right.
> 
> What we're trying to do here is only consider looking at the
> capabilities if the RC Core is enabled.  If it is not enabled, the #if
> still does the right thing and makes sure that the caps are updated.
> 
> Please take another look at the semantics.

Ah, yes. You are right. But so am I: the code is just unnecessarily confusing
as is seen by this discussion.

I still would like to see a patch with my proposed solution. The control flow
is much easier to understand that way.

Regards,

	Hans

Re: [PATCH 1/2] [media] cec: Move capability check inside #if

[Lee Jones]

On Tue, 04 Apr 2017, Hans Verkuil wrote:

> On 04/04/2017 03:01 PM, Lee Jones wrote:
> > On Tue, 04 Apr 2017, Lee Jones wrote:
> > 
> >> On Tue, 04 Apr 2017, Hans Verkuil wrote:
> >>
> >>> On 04/04/2017 02:32 PM, Lee Jones wrote:
> >>>> If CONFIG_RC_CORE is not enabled then none of the RC code will be
> >>>> executed anyway, so we're placing the capability check inside the
> >>>>
> >>>> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> >>>> ---
> >>>>  drivers/media/cec/cec-core.c | 2 +-
> >>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/media/cec/cec-core.c b/drivers/media/cec/cec-core.c
> >>>> index 37217e2..06a312c 100644
> >>>> --- a/drivers/media/cec/cec-core.c
> >>>> +++ b/drivers/media/cec/cec-core.c
> >>>> @@ -234,10 +234,10 @@ struct cec_adapter *cec_allocate_adapter(const struct cec_adap_ops *ops,
> >>>>  		return ERR_PTR(res);
> >>>>  	}
> >>>>  
> >>>> +#if IS_REACHABLE(CONFIG_RC_CORE)
> >>>>  	if (!(caps & CEC_CAP_RC))
> >>>>  		return adap;
> >>>>  
> >>>> -#if IS_REACHABLE(CONFIG_RC_CORE)
> >>>>  	/* Prepare the RC input device */
> >>>>  	adap->rc = rc_allocate_device(RC_DRIVER_SCANCODE);
> >>>>  	if (!adap->rc) {
> >>>>
> >>>
> >>> Not true, there is an #else further down.
> >>
> >> I saw the #else.  It's inert code that becomes function-less.
> >>
> >>> That said, this code is clearly a bit confusing.
> >>>
> >>> It would be better if at the beginning of the function we'd have this:
> >>>
> >>> #if !IS_REACHABLE(CONFIG_RC_CORE)
> >>> 	caps &= ~CEC_CAP_RC;
> >>> #endif
> >>>
> >>> and then drop the #else bit and (as you do in this patch) move the #if up.
> >>>
> >>> Can you make a new patch for this?
> >>
> >> Sure.
> > 
> > No wait, sorry!  This patch is the correct fix.
> > 
> > 'caps' is already indicating !CEC_CAP_RC, which is right.
> > 
> > What we're trying to do here is only consider looking at the
> > capabilities if the RC Core is enabled.  If it is not enabled, the #if
> > still does the right thing and makes sure that the caps are updated.
> > 
> > Please take another look at the semantics.
> 
> Ah, yes. You are right. But so am I: the code is just unnecessarily confusing
> as is seen by this discussion.
> 
> I still would like to see a patch with my proposed solution. The control flow
> is much easier to understand that way.

I have an idea.  Please bear with me.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 1/2] [media] cec: Move capability check inside #if

[Lee Jones]

On Tue, 04 Apr 2017, Hans Verkuil wrote:

> On 04/04/2017 03:01 PM, Lee Jones wrote:
> > On Tue, 04 Apr 2017, Lee Jones wrote:
> > 
> >> On Tue, 04 Apr 2017, Hans Verkuil wrote:
> >>
> >>> On 04/04/2017 02:32 PM, Lee Jones wrote:
> >>>> If CONFIG_RC_CORE is not enabled then none of the RC code will be
> >>>> executed anyway, so we're placing the capability check inside the
> >>>>
> >>>> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> >>>> ---
> >>>>  drivers/media/cec/cec-core.c | 2 +-
> >>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/media/cec/cec-core.c b/drivers/media/cec/cec-core.c
> >>>> index 37217e2..06a312c 100644
> >>>> --- a/drivers/media/cec/cec-core.c
> >>>> +++ b/drivers/media/cec/cec-core.c
> >>>> @@ -234,10 +234,10 @@ struct cec_adapter *cec_allocate_adapter(const struct cec_adap_ops *ops,
> >>>>  		return ERR_PTR(res);
> >>>>  	}
> >>>>  
> >>>> +#if IS_REACHABLE(CONFIG_RC_CORE)
> >>>>  	if (!(caps & CEC_CAP_RC))
> >>>>  		return adap;
> >>>>  
> >>>> -#if IS_REACHABLE(CONFIG_RC_CORE)
> >>>>  	/* Prepare the RC input device */
> >>>>  	adap->rc = rc_allocate_device(RC_DRIVER_SCANCODE);
> >>>>  	if (!adap->rc) {
> >>>>
> >>>
> >>> Not true, there is an #else further down.
> >>
> >> I saw the #else.  It's inert code that becomes function-less.
> >>
> >>> That said, this code is clearly a bit confusing.
> >>>
> >>> It would be better if at the beginning of the function we'd have this:
> >>>
> >>> #if !IS_REACHABLE(CONFIG_RC_CORE)
> >>> 	caps &= ~CEC_CAP_RC;
> >>> #endif
> >>>
> >>> and then drop the #else bit and (as you do in this patch) move the #if up.
> >>>
> >>> Can you make a new patch for this?
> >>
> >> Sure.
> > 
> > No wait, sorry!  This patch is the correct fix.
> > 
> > 'caps' is already indicating !CEC_CAP_RC, which is right.
> > 
> > What we're trying to do here is only consider looking at the
> > capabilities if the RC Core is enabled.  If it is not enabled, the #if
> > still does the right thing and makes sure that the caps are updated.
> > 
> > Please take another look at the semantics.
> 
> Ah, yes. You are right. But so am I: the code is just unnecessarily confusing
> as is seen by this discussion.
> 
> I still would like to see a patch with my proposed solution. The control flow
> is much easier to understand that way.

I have an idea.  Please bear with me.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog