Re: [PATCH] [media] cec: Handle RC capability more elegantly

[Hans Verkuil]

On 04/04/2017 04:43 PM, Lee Jones wrote:
> If a user specifies the use of RC as a capability, they should
> really be enabling RC Core code.  If they do not we WARN() them
> of this and disable the capability for them.
> 
> Once we know RC Core code has not been enabled, we can update
> the user's capabilities and use them as a term of reference for
> other RC-only calls.  This is preferable to having ugly #ifery
> scattered throughout C code.
> 
> Most of the functions are actually safe to call, since they
> sensibly check for a NULL RC pointer before they attempt to
> deference it.
> 
> Signed-off-by: Lee Jones <lee.jones@xxxxxxxxxx>
> ---
>  drivers/media/cec/cec-core.c | 19 +++++++------------
>  1 file changed, 7 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/media/cec/cec-core.c b/drivers/media/cec/cec-core.c
> index cfe414a..51be8d6 100644
> --- a/drivers/media/cec/cec-core.c
> +++ b/drivers/media/cec/cec-core.c
> @@ -208,9 +208,13 @@ struct cec_adapter *cec_allocate_adapter(const struct cec_adap_ops *ops,
>  		return ERR_PTR(-EINVAL);
>  	if (WARN_ON(!available_las || available_las > CEC_MAX_LOG_ADDRS))
>  		return ERR_PTR(-EINVAL);
> +	if (WARN_ON(caps & CEC_CAP_RC && !IS_REACHABLE(CONFIG_RC_CORE)))
> +		caps &= ~CEC_CAP_RC;

Don't use WARN_ON, this is not an error of any kind. Neither do you need to add the
'caps & CEC_CAP_RC' test. Really, it's just simpler to do what I suggested before
with an #if.

> +
>  	adap = kzalloc(sizeof(*adap), GFP_KERNEL);
>  	if (!adap)
>  		return ERR_PTR(-ENOMEM);
> +
>  	strlcpy(adap->name, name, sizeof(adap->name));
>  	adap->phys_addr = CEC_PHYS_ADDR_INVALID;
>  	adap->log_addrs.cec_version = CEC_OP_CEC_VERSION_2_0;
> @@ -237,7 +241,6 @@ struct cec_adapter *cec_allocate_adapter(const struct cec_adap_ops *ops,
>  	if (!(caps & CEC_CAP_RC))
>  		return adap;
>  
> -#if IS_REACHABLE(CONFIG_RC_CORE)

Huh? If CONFIG_RC_CORE is undefined, all these rc_ calls will fail when linking!

Regards,

	Hans

>  	/* Prepare the RC input device */
>  	adap->rc = rc_allocate_device(RC_DRIVER_SCANCODE);
>  	if (!adap->rc) {
> @@ -264,9 +267,7 @@ struct cec_adapter *cec_allocate_adapter(const struct cec_adap_ops *ops,
>  	adap->rc->priv = adap;
>  	adap->rc->map_name = RC_MAP_CEC;
>  	adap->rc->timeout = MS_TO_NS(100);
> -#else
> -	adap->capabilities &= ~CEC_CAP_RC;
> -#endif
> +
>  	return adap;
>  }
>  EXPORT_SYMBOL_GPL(cec_allocate_adapter);
> @@ -285,7 +286,6 @@ int cec_register_adapter(struct cec_adapter *adap,
>  	adap->owner = parent->driver->owner;
>  	adap->devnode.dev.parent = parent;
>  
> -#if IS_REACHABLE(CONFIG_RC_CORE)
>  	if (adap->capabilities & CEC_CAP_RC) {
>  		adap->rc->dev.parent = parent;
>  		res = rc_register_device(adap->rc);
> @@ -298,15 +298,13 @@ int cec_register_adapter(struct cec_adapter *adap,
>  			return res;
>  		}
>  	}
> -#endif
>  
>  	res = cec_devnode_register(&adap->devnode, adap->owner);
>  	if (res) {
> -#if IS_REACHABLE(CONFIG_RC_CORE)
>  		/* Note: rc_unregister also calls rc_free */
>  		rc_unregister_device(adap->rc);
>  		adap->rc = NULL;
> -#endif
> +
>  		return res;
>  	}
>  
> @@ -337,11 +335,10 @@ void cec_unregister_adapter(struct cec_adapter *adap)
>  	if (IS_ERR_OR_NULL(adap))
>  		return;
>  
> -#if IS_REACHABLE(CONFIG_RC_CORE)
>  	/* Note: rc_unregister also calls rc_free */
>  	rc_unregister_device(adap->rc);
>  	adap->rc = NULL;
> -#endif
> +
>  	debugfs_remove_recursive(adap->cec_dir);
>  	cec_devnode_unregister(&adap->devnode);
>  }
> @@ -357,9 +354,7 @@ void cec_delete_adapter(struct cec_adapter *adap)
>  	kthread_stop(adap->kthread);
>  	if (adap->kthread_config)
>  		kthread_stop(adap->kthread_config);
> -#if IS_REACHABLE(CONFIG_RC_CORE)
>  	rc_free_device(adap->rc);
> -#endif
>  	kfree(adap);
>  }
>  EXPORT_SYMBOL_GPL(cec_delete_adapter);
>