From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933157AbdDEJNF (ORCPT ); Wed, 5 Apr 2017 05:13:05 -0400 Received: from mail-wm0-f47.google.com ([74.125.82.47]:36082 "EHLO mail-wm0-f47.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933027AbdDEJNC (ORCPT ); Wed, 5 Apr 2017 05:13:02 -0400 Date: Wed, 5 Apr 2017 10:12:58 +0100 From: Lee Jones To: Hans Verkuil Cc: Russell King - ARM Linux , benjamin.gaignard@st.com, kernel@stlinux.com, patrice.chotard@st.com, linux-kernel@vger.kernel.org, hans.verkuil@cisco.com, mchehab@kernel.org, linux-arm-kernel@lists.infradead.org, linux-media@vger.kernel.org Subject: Re: [PATCH] [media] cec: Handle RC capability more elegantly Message-ID: <20170405091258.zujipcybjeh37amq@dell> References: <20170404144309.31357-1-lee.jones@linaro.org> <9fdac3c1-b249-839e-c2bc-f4661994eb3a@xs4all.nl> <20170404151939.bvd252nprj6kjmdu@dell> <20170404153659.GC7909@n2100.armlinux.org.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.6.2-neo (2016-08-21) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > >>>> --- > >>>> 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