Hi Shardar,
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.
Best regards,
Valentin
On Wed, Nov 16, 2016 at 08:31:35AM +0100, Valentin Rothberg wrote:
> Hi Shardar,
>
> 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.
This looks to be something that was part of some downstream kernel a
long time ago. Given that it doesn't exist upstream and the driver does
not use anything related to it, I'll just drop the select. Thanks for
pointing this out.
Thierry
On Wed, Nov 16, 2016 at 04:39:57PM +0100, Thierry Reding wrote:
> On Wed, Nov 16, 2016 at 08:31:35AM +0100, Valentin Rothberg wrote:
> > Hi Shardar,
> >
> > 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.
>
> This looks to be something that was part of some downstream kernel a
> long time ago. Given that it doesn't exist upstream and the driver does
> not use anything related to it, I'll just drop the select. Thanks for
> pointing this out.
Actually the I2C_ALGO_BIT seems to be completely unused as well. I'm
going to drop that, too.
Thierry
> On Wed, Nov 16, 2016 at 04:39:57PM +0100, Thierry Reding wrote:
> > On Wed, Nov 16, 2016 at 08:31:35AM +0100, Valentin Rothberg wrote:
> > > Hi Shardar,
> > >
> > > 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.
> >
> > This looks to be something that was part of some downstream kernel a
> > long time ago. Given that it doesn't exist upstream and the driver
> > does not use anything related to it, I'll just drop the select. Thanks
> > for pointing this out.
>
> Actually the I2C_ALGO_BIT seems to be completely unused as well. I'm going
> to drop that, too.
>
Yes as Thierry mentioned both I2C_ALGO_BUSCLEAR and I2C_ALGO_BIT can be removed as these are obsolete.
[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 <[email protected]>
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 <[email protected]>
Acked-by: Michal Marek <[email protected]> (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
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 <[email protected]>
> 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 <[email protected]>
> Acked-by: Michal Marek <[email protected]> (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
Hi Valentin,
On Thu, 2016-11-17 at 12:33 +0100, Valentin Rothberg wrote:
> I tested your patch and it works properly for me. It even still applies
> on linux-next.
Cool.
(We should probably compile a list of current invalid selects somewhere
publicly, and team up to submit patches for the low hanging fruit (ie, obvious
typos) and for the arches that actually matter before I resubmit.)
> Would it be possible to extend your patch to also check
> symbols in other statements (depends on, if, etc.)?
No. Depending on an unknown symbol is common and correct. Think
depends on SOME_OBSCURE_ARM_BOARD
somewhere in drivers. Only arm builds will ever know about the
SOME_OBSCURE_ARM_BOARD symbol.
What could be done is starting any "make *config" target with a treewide pass
over all Kconfig files to collect all valid symbols and so spot typos and
other obviously incorrect. The example that comes to mind is
depends on SH
that I have spotted a few times in the past years. But this treewide pass will
incur some runtime cost and might not be easy to implement cleanly. Perhaps
we're better of with using your script for that.
Thanks,
Paul Bolle
Hi Paul,
On Thu, Nov 17, 2016 at 12:55 PM, Paul Bolle <[email protected]> wrote:
> Hi Valentin,
>
> On Thu, 2016-11-17 at 12:33 +0100, Valentin Rothberg wrote:
>> I tested your patch and it works properly for me. It even still applies
>> on linux-next.
>
> Cool.
>
> (We should probably compile a list of current invalid selects somewhere
> publicly, and team up to submit patches for the low hanging fruit (ie, obvious
> typos) and for the arches that actually matter before I resubmit.)
Sounds like a plan. At the moment I really don't have much time, but
starting in mid December I could help. I propose to generate a bunch
of (rand)configs for different arches and collect the warnings with
your patch. After we just need to compare it with the symbols from
checkkconfigsymbols.
>> Would it be possible to extend your patch to also check
>> symbols in other statements (depends on, if, etc.)?
>
> No. Depending on an unknown symbol is common and correct. Think
> depends on SOME_OBSCURE_ARM_BOARD
>
> somewhere in drivers. Only arm builds will ever know about the
> SOME_OBSCURE_ARM_BOARD symbol.
You're completely right, thanks. Obviously I was too lazy to think about it.
> What could be done is starting any "make *config" target with a treewide pass
> over all Kconfig files to collect all valid symbols and so spot typos and
> other obviously incorrect. The example that comes to mind is
> depends on SH
>
> that I have spotted a few times in the past years. But this treewide pass will
> incur some runtime cost and might not be easy to implement cleanly. Perhaps
> we're better of with using your script for that.
I agree.
Kind regards,
Valentin