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

[Russell King - ARM Linux]

On Tue, Apr 04, 2017 at 04:19:39PM +0100, Lee Jones wrote:
> 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.

Please print warning using pr_warn() or dev_warn().  Using WARN_ON()
because something is not configured is _really_ not nice behaviour.
Consider how useful a stack trace is to the user for this situation -
it's completely meaningless.

A message that prompts the user to enable RC_CORE would make more sense,
and be much more informative to the user.  Maybe something like this:

+	if (caps & CEC_CAP_RC && !IS_REACHABLE(CONFIG_RC_CORE)) {
+		pr_warn("CEC: driver %pf requests RC, please enable CONFIG_RC_CORE\n",
+			__builtin_return_address(0));
+		caps &= ~CEC_CAP_RC;
+	}

It could be much more informative by using dev_warn() if we had the
'struct device' passed in to this function, and then we wouldn't need
to use __builtin_return_address().

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.