On Tue, 04 Apr 2017, Hans Verkuil wrote:
> 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.
Right, this is not an error.
That's why we are warning the user instead of bombing out.
> 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.
This does exactly what you asked.
Just to clarify, can you explain to me when asking for RC support, but
not enabling it would ever be a valid configuration?
> > +
> > 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!
I thought I'd tested for that, but it turns out that *my*
CONFIG_RC_CORE=n config was being over-ridden by the build system.
If it will really fail when linking, it sounds like the RC subsystem
is not written properly. I guess that explains why all these drivers
are riddled with ugly #ifery.
Will fix that too, bear with.
> > /* 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);
> >
>
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
