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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20170405091258.zujipcybjeh37amq@dell>
Date:   Wed, 5 Apr 2017 10:12:58 +0100
From:   Lee Jones <lee.jones@...aro.org>
To:     Hans Verkuil <hverkuil@...all.nl>
Cc:     Russell King - ARM Linux <linux@...linux.org.uk>,
        benjamin.gaignard@...com, kernel@...inux.com,
        patrice.chotard@...com, linux-kernel@...r.kernel.org,
        hans.verkuil@...co.com, mchehab@...nel.org,
        linux-arm-kernel@...ts.infradead.org, linux-media@...r.kernel.org
Subject: Re: [PATCH] [media] cec: Handle RC capability more elegantly

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@...aro.org>
> >>>> ---
> >>>>  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