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

[Lee Jones]

On Tue, 04 Apr 2017, Russell King - ARM Linux wrote:

> On Tue, Apr 04, 2017 at 05:10:05PM +0100, Lee Jones wrote:
> > @@ -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)
> >  	/* Prepare the RC input device */
> >  	adap->rc = rc_allocate_device(RC_DRIVER_SCANCODE);
> >  	if (!adap->rc) {
> 
> The above, coupled with patch 1:
> 
> +#ifdef CONFIG_RC_CORE
>  struct rc_dev *rc_allocate_device(enum rc_driver_type);
> +#else
> +static inline struct rc_dev *rc_allocate_device(int unused)
> +{
> +       return ERR_PTR(-EOPNOTSUPP);
> +}
> +#endif
> 
> is really not nice.  You claim that this is how stuff is done elsewhere
> in the kernel, but no, it isn't.  Look at debugfs.

I'm afraid you have entered half way through a conversation, which as
caused a misunderstanding.  Apologies for not being clear in my commit
log.

When I say "this is how it's done else where", that is in reference to
offering stubs which can be tucked away in a header file, rather than
being forced to #if out any functionality which is not available.

> You're right that debugfs returns an error pointer when it's not
> configured.  However, the debugfs dentry is only ever passed back into
> the debugfs APIs, it is never dereferenced by the caller.

Continued on from my last point:

What I do not mean is that this solution is perfect and does not
require a review.  You are completely correct in what you say, the
return values I have used are not suitable.  I failed to see how
callers were treating the return value.  I will carry out due
diligence on that point and re-submit as per your request.

> That is not the case here.  The effect if your change is that the
> following dereferences will oops the kernel.  This is unacceptable for
> a feature that is deconfigured.

Fair point Russell.  Thanks, will fix.

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