Date: Thu, 17 Nov 2016 12:33:01 +0100
From: Valentin Rothberg <valentinrothberg@gmail.com>
To: Paul Bolle <pebolle@tiscali.nl>
Cc: smohammed@nvidia.com, treding@nvidia.com,
        Michal Marek <mmarek@suse.cz>, linux-kernel@vger.kernel.org
Subject: Re: i2c: undefined option I2C_ALGO_BUSCLEAR
Message-ID: <20161117113301.qqce6a4kmmsv6ijl@tumbleweed>
References: <CAD3Xx4+ieNPqsumoXuiTtpwxct-66ipJB8tb-Kfpe1njBc+OZQ@mail.gmail.com>
 <1479373111.19539.24.camel@tiscali.nl>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <1479373111.19539.24.camel@tiscali.nl>
User-Agent: Mutt/1.6.2 (2016-07-01)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4125
Lines: 125

Hi Paul,

I tested your patch and it works properly for me.  It even still applies
on linux-next.  Would it be possible to extend your patch to also check
symbols in other statements (depends on, if, etc.)?

Best regards,
 Valentin

On Nov 17 '16 09:58, Paul Bolle wrote:
> [Added Michal]
> 
> Hi Valentin,
> 
> On Wed, 2016-11-16 at 08:31 +0100, Valentin Rothberg wrote:
> > your commit c3ca951fe41a ("i2c: Add Tegra BPMP I2C proxy driver")
> > popped up in today's linux-next tree, adding Kconfig option
> > I2C_TEGRA_BPMP, which further selects I2C_ALGO_BUSCLEAR, which is
> > nowhere defined in Kconfig.
> > 
> > Is there a patch queued somewhere to add I2C_ALGO_BUSCLEAR to Kconfig?
> >  I could not find anything on the lkml; only some older repositories
> > on github, where the options is defined in drivers/i2c/busses/Kconfig.
> 
> Attached is a v2 of a patch I submitted way to long ago. It adds a warning for
> exactly this issue. That should help others to spot them. (It's a bit smarter
> than your check kconfig script as it also warns if you select symbols that are
> outside of you arch's scope. Ie, not only for entirely unknown symbols.)
> 
> Please play with it. Unless you spot some issues I really should be properly
> resubmitting it.
> 
> Thanks,
> 
> 
> Paul Bolle
> 
> -------- >8 >8 >8 >8 -------
> From: Paul Bolle <pebolle@tiscali.nl>
> Subject: [PATCH] kconfig: warn if an unknown symbol is selected
> 
> A select of an unknown symbol is basically treated as a nop and is
> silently skipped. This is annoying if the selected symbol contains a
> typo. It can also hide the fact that a treewide symbol cleanup was only
> done partially.
> 
> There are also a few cases were this might have been done on purpose.
> But that anti-pattern should be discouraged. Almost all select
> statements point to a known and reachable symbol. So people will likely
> assume that any selected symbol is actually set. Selects that violate
> this assumption can only be spotted by checking multiple Kconfig files,
> often across architectures. It is unlikely that people will do this
> regularily.
> 
> So let's warn when we notice a select of a symbol that is not known in
> the configuration we're creating.
> 
> Signed-off-by: Paul Bolle <pebolle@tiscali.nl>
> Acked-by: Michal Marek <mmarek@suse.cz> (v1)
> ---
>  scripts/kconfig/conf.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/scripts/kconfig/conf.c b/scripts/kconfig/conf.c
> index 866369f10ff8..55a4dacb196d 100644
> --- a/scripts/kconfig/conf.c
> +++ b/scripts/kconfig/conf.c
> @@ -447,6 +447,46 @@ static void check_conf(struct menu *menu)
>  		check_conf(child);
>  }
>  
> +static void check_selects(struct menu *menu)
> +{
> +	struct symbol *sym, *sel;
> +	struct property *prop;
> +
> +	while (menu) {
> +		sym = menu->sym;
> +
> +		if (sym && !sym_is_choice(sym) &&
> +		    sym->type != S_UNKNOWN &&
> +		    sym_get_tristate_value(sym) != no &&
> +		    sym->flags & SYMBOL_WRITE) {
> +			for_all_properties(sym, prop, P_SELECT) {
> +				sel = prop->expr->left.sym;
> +				if (sel->type == S_UNKNOWN &&
> +				    expr_calc_value(prop->visible.expr) != no) {
> +					fprintf(stderr, "%s:%d:warning: ",
> +						prop->file->name,
> +						prop->lineno);
> +					fprintf(stderr,
> +						"'%s' selects unknown symbol '%s'\n",
> +						sym->name,
> +						sel->name);
> +				}
> +			}
> +		}
> +
> +		if (menu->list) {
> +			menu = menu->list;
> +		} else if (menu->next) {
> +			menu = menu->next;
> +		} else while ((menu = menu->parent)) {
> +			if (menu->next) {
> +				menu = menu->next;
> +				break;
> +			}
> +		}
> +	}
> +}
> +
>  static struct option long_opts[] = {
>  	{"oldaskconfig",    no_argument,       NULL, oldaskconfig},
>  	{"oldconfig",       no_argument,       NULL, oldconfig},
> @@ -686,6 +726,8 @@ int main(int ac, char **av)
>  		break;
>  	}
>  
> +	check_selects(rootmenu.list);
> +
>  	if (sync_kconfig) {
>  		/* silentoldconfig is used during the build so we shall update autoconf.
>  		 * All other commands are only used to generate a config.
> -- 
> 2.7.4