Re: [PATCH] kconfig: Minimize 'Selected by' and 'Implied by'

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sun, Feb 4, 2018 at 4:50 PM, Petr Vorel <petr.vorel@xxxxxxxxx> wrote:
> Hi Eugeniu,
>
>> From: Eugeniu Rosca <erosca@xxxxxxxxxxxxxx>
>
>> Commit 1ccb27143360 ("kconfig: make "Selected by:" and "Implied by:"
>> readable") made an incredible improvement in how reverse dependencies
>> are perceived by the user, by breaking down the single (often
>> interminable) expression string into small readable chunks.
>
>> Even so, what happens in practice when reading the reverse
>> dependencies is that 80-90% of the OR sub-expressions simply don't
>> matter, since they evaluate to [=n].
>
>> Assuming commit 617aebe6a97e ("Merge tag 'usercopy-v4.16-rc1' of
>> git://git.kernel.org/pub/scm/linux/kernel/git/kees/linux"), ARCH=arm64
>> and vanilla arm64 defconfig, here is the top 30 of CONFIG options with
>> the highest amount of OR sub-expressions that make up the final
>> "{Selected,Implied} by" reverse dependency expression.
>
>> | Config                        | Revdep all | Revdep ![=n] |
>> |-------------------------------|------------|--------------|
>> | REGMAP_I2C                    | 212        | 9            |
>> | CRC32                         | 167        | 25           |
>> | FW_LOADER                     | 128        | 5            |
>> | MFD_CORE                      | 124        | 9            |
>> | FB_CFB_IMAGEBLIT              | 114        | 2            |
>> | FB_CFB_COPYAREA               | 111        | 2            |
>> | FB_CFB_FILLRECT               | 110        | 2            |
>> | SND_PCM                       | 103        | 2            |
>> | CRYPTO_HASH                   | 87         | 19           |
>> | WATCHDOG_CORE                 | 86         | 6            |
>> | IRQ_DOMAIN                    | 75         | 19           |
>> | SERIAL_CORE                   | 75         | 9            |
>> | PHYLIB                        | 74         | 16           |
>> | REGMAP_MMIO                   | 72         | 15           |
>> | GENERIC_PHY                   | 67         | 20           |
>> | DMA_ENGINE                    | 66         | 11           |
>> | SERIAL_CORE_CONSOLE           | 64         | 9            |
>> | CRYPTO_BLKCIPHER              | 64         | 13           |
>> | PINMUX                        | 60         | 17           |
>> | CRYPTO                        | 59         | 10           |
>> | MII                           | 58         | 8            |
>> | GPIOLIB_IRQCHIP               | 58         | 9            |
>> | MFD_SYSCON                    | 58         | 15           |
>> | VIDEOBUF2_DMA_CONTIG          | 46         | 4            |
>> | REGMAP_IRQ                    | 45         | 6            |
>> | REGMAP_SPI                    | 44         | 2            |
>> | CLKSRC_MMIO                   | 42         | 5            |
>> | SND_SOC_GENERIC_DMAENGINE_PCM | 41         | 3            |
>> | CRYPTO_SHA1                   | 37         | 2            |
>> | REGMAP                        | 36         | 4            |
>
>> The story behind the above is that we still need to visually
>> review/evaluate 212 expressions which *potentially* select REGMAP_I2C
>> in order to identify the expressions which *actually* select
>> REGMAP_I2C, for a particular ARCH and for a particular defconfig used.
>
>> This patch attempts to bring at user's fingertips those reverse
>> dependencies that actually participate in selection of given symbol
>> filtering out the rest of them.
>
>> Signed-off-by: Eugeniu Rosca <erosca@xxxxxxxxxxxxxx>
>> ---
>>  scripts/kconfig/expr.c | 24 +++++++++++++++++-------
>>  1 file changed, 17 insertions(+), 7 deletions(-)
>
>> diff --git a/scripts/kconfig/expr.c b/scripts/kconfig/expr.c
>> index 2ba332b3fed7..147b2d8a8f3e 100644
>> --- a/scripts/kconfig/expr.c
>> +++ b/scripts/kconfig/expr.c
>> @@ -1234,14 +1234,24 @@ static void __expr_print(struct expr *e, void (*fn)(void *, struct symbol *, con
>>               fn(data, e->right.sym, e->right.sym->name);
>>               break;
>>       case E_OR:
>> -             if (revdep && e->left.expr->type != E_OR)
>> -                     fn(data, NULL, "\n  - ");
>> -             __expr_print(e->left.expr, fn, data, E_OR, revdep);
>> -             if (revdep)
>> -                     fn(data, NULL, "\n  - ");
>> -             else
>> +             if (revdep) {
>> +                     struct expr *left = e->left.expr;
>> +                     struct expr *right = e->right.expr;
>> +
>> +                     if (expr_calc_value(left) != no) {
>> +                             if (left->type != E_OR)
>> +                                     fn(data, NULL, "\n  - ");
>> +                             __expr_print(left, fn, data, E_OR, revdep);
>> +                     }
>> +                     if (expr_calc_value(right) != no) {
>> +                             fn(data, NULL, "\n  - ");
>> +                             __expr_print(right, fn, data, E_OR, revdep);
>> +                     }
>> +             } else {
>> +                     __expr_print(e->left.expr, fn, data, E_OR, revdep);
>>                       fn(data, NULL, " || ");
>> -             __expr_print(e->right.expr, fn, data, E_OR, revdep);
>> +                     __expr_print(e->right.expr, fn, data, E_OR, revdep);
>> +             }
>>               break;
>>       case E_AND:
>>               expr_print(e->left.expr, fn, data, E_AND);
>
> Thanks for trying to improve my change.
>
> Applying your patch, running
> $ ARCH=arm64 make defconfig && ARCH=arm64 make menuconfig
> and searching for USB prints "Selected by:" with nothing actually selected.
>
> Kind regards,
> Petr

Hello,

I implemented the behavior I had in mind. With this patch, REGMAP_I2C
shows up as follows:

Symbol: REGMAP_I2C [=y]│
Type  : tristate│
  Defined at drivers/base/regmap/Kconfig:19
  Depends on: I2C [=y]
  Currently selected by:
  - RTC_I2C_AND_SPI [=y] && RTC_CLASS [=y] && I2C [=y]
  - USB_HSIC_USB3503 [=y] && USB_SUPPORT [=y] && USB [=y] && I2C [=y]
  - SND_SOC [=y] && SOUND [=y] && !UML && SND [=y] && I2C [=y]
  - DRM_I2C_ADV7511 [=m] && HAS_IOMEM [=y] && DRM [=m] && DRM_BRIDGE
[=y] && OF [=y]
  - REGULATOR_FAN53555 [=y] && REGULATOR [=y] && I2C [=y]
  - MFD_SEC_CORE [=y] && HAS_IOMEM [=y] && I2C [=y]=y
  - MFD_RK808 [=y] && HAS_IOMEM [=y] && I2C [=y] && OF [=y]
  - MFD_MAX77620 [=y] && HAS_IOMEM [=y] && I2C [=y]=y && (OF [=y] ||
COMPILE_TEST [=n])
  - SENSORS_INA2XX [=m] && HWMON [=y] && I2C [=y]
  All selecting symbols:
  - PWM_PCA9685 [=n] && PWM [=y] && I2C [=y]
  - SX9500 [=n] && IIO [=y] && I2C [=y]
  - ZPA2326_I2C [=n] && IIO [=y]
  - HP03 [=n] && IIO [=y] && I2C [=y]
  - BMP280_I2C [=n] && IIO [=y] && BMP280 [=n] && I2C [=y]
  ...

USB shows up as:

Symbol: USB [=y]
Type  : tristate
Prompt: Support for Host-side USB
  Location:
    -> Device Drivers
(1)   -> USB support (USB_SUPPORT [=y])
  Defined at drivers/usb/Kconfig:38
  Depends on: USB_SUPPORT [=y] && USB_ARCH_HAS_HCD [=y]
  Selects: USB_COMMON [=y] && NLS [=y]
  Currently selected by:
    No symbols
  All selecting symbols:
  - DRM_UDL [=n] && HAS_IOMEM [=y] && DRM [=m] && USB_SUPPORT [=y] &&
USB_ARCH_HAS_HCD [=y]
  - IR_TTUSBIR [=n] && RC_DEVICES [=y] && USB_ARCH_HAS_HCD [=y] && RC_CORE [=m]
  - IR_IGUANA [=n] && RC_DEVICES [=y] && USB_ARCH_HAS_HCD [=y] && RC_CORE [=m]
  - IR_IGORPLUGUSB [=n] && RC_DEVICES [=y] && USB_ARCH_HAS_HCD [=y] &&
RC_CORE [=m]
  - IR_STREAMZAP [=n] && RC_DEVICES [=y] && USB_ARCH_HAS_HCD [=y] &&
RC_CORE [=m]
  ...

What do you think about this behavior? Tell me if you spot any bugs.

While working on the patch, I thought I had a regression where a
reverse dependency of just a single symbol showed up without a leading
"  - ", hence the slight overengineering. On a second look, I think
the original patch also did that though. It's arguable whether that's
actually a bug, but now it always prints the "  - ".

I don't have access to Mutt at the moment, and GMail's web interface
mangles patches, so I'll attach the patch.

If you go with something similar, you could add a Suggested-by:.

Cheers,
Ulf
From 8d3e34dbf4eb11f492519464031cd610ad866b53 Mon Sep 17 00:00:00 2001
From: Ulf Magnusson <ulfalizer@xxxxxxxxx>
Date: Sun, 4 Feb 2018 18:52:40 +0100
Subject: [PATCH] Readable rev. dep WIP

---
 scripts/kconfig/expr.c | 97 +++++++++++++++++++++++++++++++++++++++++---------
 scripts/kconfig/expr.h |  1 +
 scripts/kconfig/menu.c | 11 +++++-
 3 files changed, 92 insertions(+), 17 deletions(-)

diff --git a/scripts/kconfig/expr.c b/scripts/kconfig/expr.c
index 2ba332b3fed7..fd4814cb1a31 100644
--- a/scripts/kconfig/expr.c
+++ b/scripts/kconfig/expr.c
@@ -1179,7 +1179,33 @@ struct expr *expr_simplify_unmet_dep(struct expr *e1, struct expr *e2)
 	return expr_get_leftmost_symbol(ret);
 }
 
-static void __expr_print(struct expr *e, void (*fn)(void *, struct symbol *, const char *), void *data, int prevtoken, bool revdep)
+enum expr_print_mode {
+	PRINT_NORMAL,
+
+	/* Print reverse dependencies with selects on separate lines */
+	PRINT_ALL_SELECTS,
+
+	/* Like PRINT_ALL_SELECTS, but skip 'n'-valued selects */
+	PRINT_ACTIVE_SELECTS
+};
+
+/* Helper for pretty-printing reverse dependencies with one select per line */
+static void print_select(struct expr *e,
+			 void (*fn)(void *, struct symbol *, const char *),
+			 void *data,
+			 enum expr_print_mode mode)
+{
+	if (mode == PRINT_ALL_SELECTS || expr_calc_value(e) != no) {
+		fn(data, NULL, "\n  - ");
+		expr_print(e, fn, data, E_OR);
+	}
+}
+
+static void __expr_print(struct expr *e,
+			 void (*fn)(void *, struct symbol *, const char *),
+			 void *data,
+			 int prevtoken,
+			 enum expr_print_mode mode)
 {
 	if (!e) {
 		fn(data, NULL, "y");
@@ -1190,10 +1216,18 @@ static void __expr_print(struct expr *e, void (*fn)(void *, struct symbol *, con
 		fn(data, NULL, "(");
 	switch (e->type) {
 	case E_SYMBOL:
-		if (e->left.sym->name)
-			fn(data, e->left.sym, e->left.sym->name);
-		else
+		if (e->left.sym->name) {
+			if (mode == PRINT_NORMAL)
+				fn(data, e->left.sym, e->left.sym->name);
+			else
+				/*
+				 * PRINT_*_SELECTS, with a final select that
+				 * has no condition
+				 */
+				print_select(e, fn, data, mode);
+		} else {
 			fn(data, NULL, "<choice>");
+		}
 		break;
 	case E_NOT:
 		fn(data, NULL, "!");
@@ -1234,19 +1268,42 @@ static void __expr_print(struct expr *e, void (*fn)(void *, struct symbol *, con
 		fn(data, e->right.sym, e->right.sym->name);
 		break;
 	case E_OR:
-		if (revdep && e->left.expr->type != E_OR)
-			fn(data, NULL, "\n  - ");
-		__expr_print(e->left.expr, fn, data, E_OR, revdep);
-		if (revdep)
-			fn(data, NULL, "\n  - ");
-		else
+		if (mode == PRINT_NORMAL) {
+			expr_print(e->left.expr, fn, data, E_OR);
 			fn(data, NULL, " || ");
-		__expr_print(e->right.expr, fn, data, E_OR, revdep);
+			expr_print(e->right.expr, fn, data, E_OR);
+		} else {
+			/*
+			 * PRINT_*_SELECTS
+			 *
+			 * Support both (OR, <select 1>, (OR, <select 2>)) and
+			 * (OR, (OR, <select 1>), <select 2>) format for
+			 * selects. I think you always get the second one in
+			 * practice as of writing.
+			 */
+			if (e->right.expr->type != E_OR) {
+				print_select(e->right.expr, fn, data, mode);
+				/* Move on to next select */
+				__expr_print(e->left.expr, fn, data, E_OR, mode);
+			} else {
+				print_select(e->left.expr, fn, data, mode);
+				/* Move on to next select */
+				__expr_print(e->right.expr, fn, data, E_OR, mode);
+			}
+		}
 		break;
 	case E_AND:
-		expr_print(e->left.expr, fn, data, E_AND);
-		fn(data, NULL, " && ");
-		expr_print(e->right.expr, fn, data, E_AND);
+		if (mode == PRINT_NORMAL) {
+			expr_print(e->left.expr, fn, data, E_AND);
+			fn(data, NULL, " && ");
+			expr_print(e->right.expr, fn, data, E_AND);
+		} else {
+			/*
+			 * PRINT_*_SELECTS, with a final select that has a
+			 * condition
+			 */
+			print_select(e, fn, data, mode);
+		}
 		break;
 	case E_LIST:
 		fn(data, e->right.sym, e->right.sym->name);
@@ -1276,7 +1333,7 @@ static void __expr_print(struct expr *e, void (*fn)(void *, struct symbol *, con
 
 void expr_print(struct expr *e, void (*fn)(void *, struct symbol *, const char *), void *data, int prevtoken)
 {
-	__expr_print(e, fn, data, prevtoken, false);
+	__expr_print(e, fn, data, prevtoken, PRINT_NORMAL);
 }
 
 static void expr_print_file_helper(void *data, struct symbol *sym, const char *str)
@@ -1331,5 +1388,13 @@ void expr_gstr_print(struct expr *e, struct gstr *gs)
  */
 void expr_gstr_print_revdep(struct expr *e, struct gstr *gs)
 {
-	__expr_print(e, expr_print_gstr_helper, gs, E_NONE, true);
+	__expr_print(e, expr_print_gstr_helper, gs, E_NONE, PRINT_ALL_SELECTS);
+}
+
+/*
+ * Like expr_gstr_print_revdep(), but do not print 'n'-valued selects
+ */
+void expr_gstr_print_active_revdep(struct expr *e, struct gstr *gs)
+{
+	__expr_print(e, expr_print_gstr_helper, gs, E_NONE, PRINT_ACTIVE_SELECTS);
 }
diff --git a/scripts/kconfig/expr.h b/scripts/kconfig/expr.h
index c16e82e302a2..c0274dc23a47 100644
--- a/scripts/kconfig/expr.h
+++ b/scripts/kconfig/expr.h
@@ -311,6 +311,7 @@ void expr_fprint(struct expr *e, FILE *out);
 struct gstr; /* forward */
 void expr_gstr_print(struct expr *e, struct gstr *gs);
 void expr_gstr_print_revdep(struct expr *e, struct gstr *gs);
+void expr_gstr_print_active_revdep(struct expr *e, struct gstr *gs);
 
 static inline int expr_is_yes(struct expr *e)
 {
diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c
index 99222855544c..170a8b421e97 100644
--- a/scripts/kconfig/menu.c
+++ b/scripts/kconfig/menu.c
@@ -827,7 +827,16 @@ static void get_symbol_str(struct gstr *r, struct symbol *sym,
 
 	get_symbol_props_str(r, sym, P_SELECT, _("  Selects: "));
 	if (sym->rev_dep.expr) {
-		str_append(r, _("  Selected by: "));
+		str_append(r, _("  Currently selected by:"));
+		if (expr_calc_value(sym->rev_dep.expr) == no) {
+		        str_append(r, "\n");
+			str_append(r, _("    No symbols"));
+		} else {
+			expr_gstr_print_active_revdep(sym->rev_dep.expr, r);
+		}
+		str_append(r, "\n");
+
+		str_append(r, _("  All selecting symbols:"));
 		expr_gstr_print_revdep(sym->rev_dep.expr, r);
 		str_append(r, "\n");
 	}
-- 
2.14.1


[Index of Archives]     [Linux&nblp;USB Development]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite Secrets]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux