linux-kernel - Re: [PATCH v3 5/7] mfd: cros_ec: Sync to the latest cros_ec

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] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 17 Jun 2014 10:53:22 +0200
From:	Paul Bolle <pebolle@...cali.nl>
To:	Doug Anderson <dianders@...omium.org>
Cc:	Bill Richardson <wfrichar@...omium.org>,
	Simon Glass <sjg@...omium.org>,
	Lee Jones <lee.jones@...aro.org>,
	Stephen Warren <swarren@...dia.com>,
	Wolfram Sang <wsa@...-dreams.de>,
	Andrew Bresticker <abrestic@...omium.org>,
	Dylan Reid <dgreid@...omium.org>,
	Olof Johansson <olof@...om.net>,
	Samuel Ortiz <sameo@...ux.intel.com>,
	linux-samsung-soc <linux-samsung-soc@...r.kernel.org>,
	"linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 5/7] mfd: cros_ec: Sync to the latest
 cros_ec_commands.h from EC sources

Doug,

On Fri, 2014-06-13 at 08:22 -0700, Doug Anderson wrote:
> On Fri, Jun 13, 2014 at 1:08 AM, Paul Bolle <pebolle@...cali.nl> wrote:
> > On Wed, 2014-06-11 at 08:11 -0700, Doug Anderson wrote:
> >> This is a config option on the ChromeOS EC
> >> <https://chromium.googlesource.com/chromiumos/platform/ec>.  Doing a
> >> grep there:
> >>
> >> board/samus/board.h:#define CONFIG_CHARGER_PROFILE_OVERRIDE
> >> common/charge_state_v2.c:#ifdef CONFIG_CHARGER_PROFILE_OVERRIDE
> >> common/charge_state_v2.c:#ifdef CONFIG_CHARGER_PROFILE_OVERRIDE
> >> common/charge_state_v2.c:#ifdef CONFIG_CHARGER_PROFILE_OVERRIDE
> >> driver/battery/samus.c:#ifdef CONFIG_CHARGER_PROFILE_OVERRIDE
> >> driver/battery/samus.c:#endif   /* CONFIG_CHARGER_PROFILE_OVERRIDE */
> >> include/config.h:#undef CONFIG_CHARGER_PROFILE_OVERRIDE
> >> include/ec_commands.h:  /* Range for CONFIG_CHARGER_PROFILE_OVERRIDE params */
> >> test/test_config.h:#define CONFIG_CHARGER_PROFILE_OVERRIDE
> >
> > I see. So this is not a Kconfig macro but a general macro with a CONFIG_
> > prefix. There are quite a bit of those in the tree already, but still,
> > would another prefix also do?
> 
> Given that it's an entirely separate project and this is a valid
> CONFIG option in that project, it seems a lot to ask them not to use
> the CONFIG_ prefix.  Also: the part you are objecting to is only a
> comment, right?

So all the hits you quoted above are actually from code that's never
going to be included in the kernel tree, right? If so, then yes, we're
only discussing a single comment.

> We could certainly add extra wording in the comment to make it obvious
> that this is a CONFIG option for the EC and not the kernel.  Would
> that be enough?  ...or are you trying to use some scripts to
> automatically process files to look for CONFIG options?

Yes, I'm using a script to check for Kconfig macros, among other things.
It doesn't care about comments (because every now and then mistakes are
made in comments too, and some of those can get surprisingly confusing).

Anyhow, the CONFIG_ prefix used in the kernel tree is quite generic, but
we're stuck with it. Would it be bothersome to drop it in that comment?
Mentioning a preprocessor macro from a separate project is a bit
confusing to begin with. How is one supposed to know that this is a
reference to something out of tree?

So, in summary, while we're apparently only discussing a single comment,
I would appreciate it if it could be reworded, preferably by dropping
that the CONFIG_ prefix. But other people might care very little, as
they don't share this particular pet peeve.

Thanks,


Paul Bolle

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/