On Tue, 04 Apr 2017, Hans Verkuil wrote:
> On 04/04/2017 05:36 PM, Russell King - ARM Linux wrote:
> > 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().
> >
>
> I don't want to see a message logged because of this. In the current design it
> is perfectly valid to compile without RC_CORE.
>
> I think eventually this should be redesigned a bit (a separate CEC config option
> that enables or disables RC support), but for now I prefer to leave this as-is
> until I have a bit more experience with this.
>
> After the CEC notifier work is in I will take another look at this.
Well at least I bought it to your attention. I guess that's a 50% win.
I'll rework the patch accordingly.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
