* Kconfig fails: big select-based circular dependency @ 2014-06-07 9:09 Russell King - ARM Linux 2014-06-12 9:47 ` Russell King - ARM Linux 0 siblings, 1 reply; 13+ messages in thread From: Russell King - ARM Linux @ 2014-06-07 9:09 UTC (permalink / raw) To: linux-arm-kernel This is getting silly: scripts/kconfig/conf --silentoldconfig Kconfig drivers/dma/Kconfig:5:error: recursive dependency detected! drivers/dma/Kconfig:5: symbol DMADEVICES is selected by SAMSUNG_DMADEV arch/arm/plat-samsung/Kconfig:412: symbol SAMSUNG_DMADEV is selected by S3C64XX_PL080 arch/arm/mach-s3c64xx/Kconfig:20: symbol S3C64XX_PL080 is selected by SPI_S3C64XX drivers/spi/Kconfig:429: symbol SPI_S3C64XX depends on SPI drivers/spi/Kconfig:8: symbol SPI is selected by DRM_PANEL_LD9040 drivers/gpu/drm/panel/Kconfig:19: symbol DRM_PANEL_LD9040 depends on DRM_PANEL drivers/gpu/drm/panel/Kconfig:1: symbol DRM_PANEL is selected by DRM_IMX_LDB drivers/staging/imx-drm/Kconfig:35: symbol DRM_IMX_LDB depends on MFD_SYSCON drivers/mfd/Kconfig:722: symbol MFD_SYSCON is selected by POWER_RESET_KEYSTONE drivers/power/reset/Kconfig:76: symbol POWER_RESET_KEYSTONE depends on POWER_SUPPLY drivers/power/Kconfig:1: symbol POWER_SUPPLY is selected by HID_SONY drivers/hid/Kconfig:622: symbol HID_SONY depends on NEW_LEDS drivers/leds/Kconfig:8: symbol NEW_LEDS is selected by BACKLIGHT_ADP8860 drivers/video/backlight/Kconfig:327: symbol BACKLIGHT_ADP8860 depends on BACKLIGHT_CLASS_DEVICE drivers/video/backlight/Kconfig:156: symbol BACKLIGHT_CLASS_DEVICE is selected by FB_MX3 drivers/video/fbdev/Kconfig:2333: symbol FB_MX3 depends on MX3_IPU drivers/dma/Kconfig:121: symbol MX3_IPU depends on DMADEVICES This is a good illustration why the select disease is truely bad... -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Kconfig fails: big select-based circular dependency 2014-06-07 9:09 Kconfig fails: big select-based circular dependency Russell King - ARM Linux @ 2014-06-12 9:47 ` Russell King - ARM Linux 2014-06-12 10:22 ` Paul Bolle 2014-06-12 10:31 ` Arnd Bergmann 0 siblings, 2 replies; 13+ messages in thread From: Russell King - ARM Linux @ 2014-06-12 9:47 UTC (permalink / raw) To: linux-arm-kernel Ping. This causes randconfig and all*config to fail without building anything. If no one responds, I'll assume that no one is interested, and I'll just create a pile of patches removing a bunch of these idiotic select statements at random to break this loop. On Sat, Jun 07, 2014 at 10:09:44AM +0100, Russell King - ARM Linux wrote: > This is getting silly: > > scripts/kconfig/conf --silentoldconfig Kconfig > drivers/dma/Kconfig:5:error: recursive dependency detected! > drivers/dma/Kconfig:5: symbol DMADEVICES is selected by SAMSUNG_DMADEV > arch/arm/plat-samsung/Kconfig:412: symbol SAMSUNG_DMADEV is selected by S3C64XX_PL080 > arch/arm/mach-s3c64xx/Kconfig:20: symbol S3C64XX_PL080 is selected by SPI_S3C64XX > drivers/spi/Kconfig:429: symbol SPI_S3C64XX depends on SPI > drivers/spi/Kconfig:8: symbol SPI is selected by DRM_PANEL_LD9040 > drivers/gpu/drm/panel/Kconfig:19: symbol DRM_PANEL_LD9040 depends on DRM_PANEL > drivers/gpu/drm/panel/Kconfig:1: symbol DRM_PANEL is selected by DRM_IMX_LDB > drivers/staging/imx-drm/Kconfig:35: symbol DRM_IMX_LDB depends on MFD_SYSCON > drivers/mfd/Kconfig:722: symbol MFD_SYSCON is selected by POWER_RESET_KEYSTONE > drivers/power/reset/Kconfig:76: symbol POWER_RESET_KEYSTONE depends on POWER_SUPPLY > drivers/power/Kconfig:1: symbol POWER_SUPPLY is selected by HID_SONY > drivers/hid/Kconfig:622: symbol HID_SONY depends on NEW_LEDS > drivers/leds/Kconfig:8: symbol NEW_LEDS is selected by BACKLIGHT_ADP8860 > drivers/video/backlight/Kconfig:327: symbol BACKLIGHT_ADP8860 depends on BACKLIGHT_CLASS_DEVICE > drivers/video/backlight/Kconfig:156: symbol BACKLIGHT_CLASS_DEVICE is selected by FB_MX3 > drivers/video/fbdev/Kconfig:2333: symbol FB_MX3 depends on MX3_IPU > drivers/dma/Kconfig:121: symbol MX3_IPU depends on DMADEVICES > > This is a good illustration why the select disease is truely bad... > > -- > FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly > improving, and getting towards what was expected from it. > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel at lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Kconfig fails: big select-based circular dependency 2014-06-12 9:47 ` Russell King - ARM Linux @ 2014-06-12 10:22 ` Paul Bolle 2014-06-12 10:49 ` Russell King - ARM Linux 2014-06-12 10:31 ` Arnd Bergmann 1 sibling, 1 reply; 13+ messages in thread From: Paul Bolle @ 2014-06-12 10:22 UTC (permalink / raw) To: linux-arm-kernel [Cc-ing Yann and linux-kbuild.] On Thu, 2014-06-12 at 10:47 +0100, Russell King - ARM Linux wrote: > If no one responds, I'll assume that no one is interested, and I'll > just create a pile of patches removing a bunch of these idiotic select > statements at random to break this loop. I looked into a much, much easier "recursive dependency" warning recently. That triggered me to post http://article.gmane.org/gmane.linux.kernel/1678946 . In summary: select statements are treated as "reverse dependencies", but that is actually rather odd. See, for example, DMADEVICES in this warning: the kconfig code treats it as if it's depending on SAMSUNG_DMADEV. But I would be very surprised if that was what was intended when that select statement was added. Please note that I have not yet really looked into the mess you reported. But for now I'll state that a recursive dependency warning involving select statements is probably bogus. Perhaps Yann e.a. can prove me wrong. I might have an ugly hack to the kconfig code disabling the "recursive dependency" stuff stashed away somewhere. Not sure, as it's been two months... Paul Bolle > On Sat, Jun 07, 2014 at 10:09:44AM +0100, Russell King - ARM Linux wrote: > > This is getting silly: > > > > scripts/kconfig/conf --silentoldconfig Kconfig > > drivers/dma/Kconfig:5:error: recursive dependency detected! > > drivers/dma/Kconfig:5: symbol DMADEVICES is selected by SAMSUNG_DMADEV > > arch/arm/plat-samsung/Kconfig:412: symbol SAMSUNG_DMADEV is selected by S3C64XX_PL080 > > arch/arm/mach-s3c64xx/Kconfig:20: symbol S3C64XX_PL080 is selected by SPI_S3C64XX > > drivers/spi/Kconfig:429: symbol SPI_S3C64XX depends on SPI > > drivers/spi/Kconfig:8: symbol SPI is selected by DRM_PANEL_LD9040 > > drivers/gpu/drm/panel/Kconfig:19: symbol DRM_PANEL_LD9040 depends on DRM_PANEL > > drivers/gpu/drm/panel/Kconfig:1: symbol DRM_PANEL is selected by DRM_IMX_LDB > > drivers/staging/imx-drm/Kconfig:35: symbol DRM_IMX_LDB depends on MFD_SYSCON > > drivers/mfd/Kconfig:722: symbol MFD_SYSCON is selected by POWER_RESET_KEYSTONE > > drivers/power/reset/Kconfig:76: symbol POWER_RESET_KEYSTONE depends on POWER_SUPPLY > > drivers/power/Kconfig:1: symbol POWER_SUPPLY is selected by HID_SONY > > drivers/hid/Kconfig:622: symbol HID_SONY depends on NEW_LEDS > > drivers/leds/Kconfig:8: symbol NEW_LEDS is selected by BACKLIGHT_ADP8860 > > drivers/video/backlight/Kconfig:327: symbol BACKLIGHT_ADP8860 depends on BACKLIGHT_CLASS_DEVICE > > drivers/video/backlight/Kconfig:156: symbol BACKLIGHT_CLASS_DEVICE is selected by FB_MX3 > > drivers/video/fbdev/Kconfig:2333: symbol FB_MX3 depends on MX3_IPU > > drivers/dma/Kconfig:121: symbol MX3_IPU depends on DMADEVICES > > > > This is a good illustration why the select disease is truely bad... ^ permalink raw reply [flat|nested] 13+ messages in thread
* Kconfig fails: big select-based circular dependency 2014-06-12 10:22 ` Paul Bolle @ 2014-06-12 10:49 ` Russell King - ARM Linux 0 siblings, 0 replies; 13+ messages in thread From: Russell King - ARM Linux @ 2014-06-12 10:49 UTC (permalink / raw) To: linux-arm-kernel On Thu, Jun 12, 2014 at 12:22:58PM +0200, Paul Bolle wrote: > [Cc-ing Yann and linux-kbuild.] > > On Thu, 2014-06-12 at 10:47 +0100, Russell King - ARM Linux wrote: > > If no one responds, I'll assume that no one is interested, and I'll > > just create a pile of patches removing a bunch of these idiotic select > > statements at random to break this loop. > > I looked into a much, much easier "recursive dependency" warning > recently. That triggered me to post > http://article.gmane.org/gmane.linux.kernel/1678946 . In summary: select > statements are treated as "reverse dependencies", but that is actually > rather odd. > > See, for example, DMADEVICES in this warning: the kconfig code treats it > as if it's depending on SAMSUNG_DMADEV. But I would be very surprised if > that was what was intended when that select statement was added. Please > note that I have not yet really looked into the mess you reported. But > for now I'll state that a recursive dependency warning involving select > statements is probably bogus. Perhaps Yann e.a. can prove me wrong. > > I might have an ugly hack to the kconfig code disabling the "recursive > dependency" stuff stashed away somewhere. Not sure, as it's been two > months... Well, let's take a look - rearranging this: symbol MX3_IPU depends on DMADEVICES symbol FB_MX3 depends on MX3_IPU So, for FB_MX3 to be enabled, we need MX3_IPU enabled and DMADEVICES also enabled. symbol BACKLIGHT_CLASS_DEVICE is selected by FB_MX3 When FB_MX3 is enabled, BACKLIGHT_CLASS_DEVICE is force-enabled. symbol BACKLIGHT_ADP8860 depends on BACKLIGHT_CLASS_DEVICE This then means that BACKLIGHT_ADP8860 can be enabled by the user, and when that is enabled by the user: symbol NEW_LEDS is selected by BACKLIGHT_ADP8860 NEW_LEDS is force-enabled. This allows HID_SONY to be enabled by the user: symbol HID_SONY depends on NEW_LEDS which in turn force-selects POWER_SUPPLY: symbol POWER_SUPPLY is selected by HID_SONY This then allows POWER_RESET_KEYSTONE to be enabled by the user: symbol POWER_RESET_KEYSTONE depends on POWER_SUPPLY which then force-selects MFD_SYSCON: symbol MFD_SYSCON is selected by POWER_RESET_KEYSTONE DRM_IMX_LDB can then be selected by the user: symbol DRM_IMX_LDB depends on MFD_SYSCON which in turn force-enables DRM_PANEL: symbol DRM_PANEL is selected by DRM_IMX_LDB allowing DRM_PANEL_LD9040 to be enabled by the user: symbol DRM_PANEL_LD9040 depends on DRM_PANEL which force-enables SPI symbol SPI is selected by DRM_PANEL_LD9040 This then allows SPI_S3C64XX to be enabled: symbol SPI_S3C64XX depends on SPI which then force-enables all these symbols, resulting in the original symbol to be enabled. symbol S3C64XX_PL080 is selected by SPI_S3C64XX symbol SAMSUNG_DMADEV is selected by S3C64XX_PL080 symbol DMADEVICES is selected by SAMSUNG_DMADEV Now, the question that Kconfig has to answer when presenting the user with the DMADEVICES option is: what are the possible legal values that this option has which the user can select? With the above loop, that can not be answered, because when DMADEVICES is 'n', the result of the above chain is that DMADEVICES is not selected, and therefore it could have values of 'y' or 'n'. However, if it were 'y' and the other options are enabled, then the only legal value of it is 'y'. So, I don't think the answer to this is to get rid of the reverse dependencies - they're very much needed to resolve what values are legal for a symbol. What this comes down to is the same old evilness - the over-use of the "select" statement on user visible symbols. For example, DMA engine is supposed to be a generic infrastructure which abstracts the DMA provider from the DMA client. That means a client driver properly converted to DMA engine does not depend on any particular DMA provider. The only dependency there is that we have a SoC where these two devices have been placed on the same silicon wafer - it is entirely possible that the SPI hardware could be placed with a different DMA engine. So, that makes SPI_S3C64XX selecting S3C64XX_PL080 wrong. I see nothing in the S3C64XX SPI driver which is specific to the S3C64XX PL080 implementation. So, that makes this select purely a "convenience" thing - something that we should /not/ be doing. That's one select statement which should be killed, and that alone will break this dependency loop. I'm sure there's other stupid select statements which do not correspond with some kind of hard dependency in the above loop which can also be killed off for the same reason. -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Kconfig fails: big select-based circular dependency 2014-06-12 9:47 ` Russell King - ARM Linux 2014-06-12 10:22 ` Paul Bolle @ 2014-06-12 10:31 ` Arnd Bergmann 2014-06-12 11:34 ` Russell King - ARM Linux 1 sibling, 1 reply; 13+ messages in thread From: Arnd Bergmann @ 2014-06-12 10:31 UTC (permalink / raw) To: linux-arm-kernel On Thursday 12 June 2014 10:47:45 Russell King - ARM Linux wrote: > If no one responds, I'll assume that no one is interested, and I'll > just create a pile of patches removing a bunch of these idiotic select > statements at random to break this loop. I missed the original mail, and I don't remember seeing this particular dependency chain. > On Sat, Jun 07, 2014 at 10:09:44AM +0100, Russell King - ARM Linux wrote: > > This is getting silly: > > > > scripts/kconfig/conf --silentoldconfig Kconfig > > drivers/dma/Kconfig:5:error: recursive dependency detected! > > drivers/dma/Kconfig:5: symbol DMADEVICES is selected by SAMSUNG_DMADEV The 'select DMADEVICES' from plat-samsung/Kconfig is certainly wrong, we shouldn't do that, but I'd do some extended build regression tests to ensure that it doesn't cause other problems. I'll have a look. > > arch/arm/plat-samsung/Kconfig:412: symbol SAMSUNG_DMADEV is selected by S3C64XX_PL080 > > arch/arm/mach-s3c64xx/Kconfig:20: symbol S3C64XX_PL080 is selected by SPI_S3C64XX > > drivers/spi/Kconfig:429: symbol SPI_S3C64XX depends on SPI > > drivers/spi/Kconfig:8: symbol SPI is selected by DRM_PANEL_LD9040 'select SPI' is also really bad. This seems trivial to replace with 'depends on SPI'. This is the only driver that uses select here. > > drivers/gpu/drm/panel/Kconfig:19: symbol DRM_PANEL_LD9040 depends on DRM_PANEL > > drivers/gpu/drm/panel/Kconfig:1: symbol DRM_PANEL is selected by DRM_IMX_LDB > > drivers/staging/imx-drm/Kconfig:35: symbol DRM_IMX_LDB depends on MFD_SYSCON > > drivers/mfd/Kconfig:722: symbol MFD_SYSCON is selected by POWER_RESET_KEYSTONE We have 10 drivers doing 'select MFD_SYSCON' and 9 drivers doing 'depends on MFD_SYSCON'. Either one would work if we did those consistently, here the problem is really mixing the two. > > drivers/power/reset/Kconfig:76: symbol POWER_RESET_KEYSTONE depends on POWER_SUPPLY > > drivers/power/Kconfig:1: symbol POWER_SUPPLY is selected by HID_SONY > > drivers/hid/Kconfig:622: symbol HID_SONY depends on NEW_LEDS > > drivers/leds/Kconfig:8: symbol NEW_LEDS is selected by BACKLIGHT_ADP8860 Here, it's much clearer: everything other than HID_SONY uses 'select NEW_LEDS', and so should this driver. > > drivers/video/backlight/Kconfig:327: symbol BACKLIGHT_ADP8860 depends on BACKLIGHT_CLASS_DEVICE > > drivers/video/backlight/Kconfig:156: symbol BACKLIGHT_CLASS_DEVICE is selected by FB_MX3 > > drivers/video/fbdev/Kconfig:2333: symbol FB_MX3 depends on MX3_IPU > > drivers/dma/Kconfig:121: symbol MX3_IPU depends on DMADEVICES > > > > This is a good illustration why the select disease is truely bad... Definitely. Arnd ^ permalink raw reply [flat|nested] 13+ messages in thread
* Kconfig fails: big select-based circular dependency 2014-06-12 10:31 ` Arnd Bergmann @ 2014-06-12 11:34 ` Russell King - ARM Linux 2014-06-12 11:57 ` Geert Uytterhoeven 2014-06-12 12:19 ` Arnd Bergmann 0 siblings, 2 replies; 13+ messages in thread From: Russell King - ARM Linux @ 2014-06-12 11:34 UTC (permalink / raw) To: linux-arm-kernel On Thu, Jun 12, 2014 at 12:31:19PM +0200, Arnd Bergmann wrote: > On Thursday 12 June 2014 10:47:45 Russell King - ARM Linux wrote: > > If no one responds, I'll assume that no one is interested, and I'll > > just create a pile of patches removing a bunch of these idiotic select > > statements at random to break this loop. > > I missed the original mail, and I don't remember seeing this particular > dependency chain. > > > On Sat, Jun 07, 2014 at 10:09:44AM +0100, Russell King - ARM Linux wrote: > > > This is getting silly: > > > > > > scripts/kconfig/conf --silentoldconfig Kconfig > > > drivers/dma/Kconfig:5:error: recursive dependency detected! > > > drivers/dma/Kconfig:5: symbol DMADEVICES is selected by SAMSUNG_DMADEV > > The 'select DMADEVICES' from plat-samsung/Kconfig is certainly wrong, > we shouldn't do that, but I'd do some extended build regression tests > to ensure that it doesn't cause other problems. > > I'll have a look. Yes, SAMSUNG_DMADEV looks like it's a shim layer between DMA engine and the old Samsung platform private DMA API. I suspect SAMSUNG_DMADEV should be selected by the drivers which make use of this shim iff DMADEVICES is enabled. > > > arch/arm/plat-samsung/Kconfig:412: symbol SAMSUNG_DMADEV is selected by S3C64XX_PL080 I think killing the select statement against S3C64XX_PL080 of this shim is also a correct move - I suspect that's just there to make Kconfig life easier (which is not really a valid reason for adding a select statement to a major subsystem.) > > > arch/arm/mach-s3c64xx/Kconfig:20: symbol S3C64XX_PL080 is selected by SPI_S3C64XX As I said in my reply to Paul, this select should also be killed. At the very least, it should be turned into "depends on S3C64XX_PL080 if ARCH_S3C64XX". Since the comments in the driver say that the driver depends on having a DMA engine, I'd suggest also adding a dependency on DMADEVICES as well, to encode that explicit requirement in the Kconfig language: config SPI_S3C64XX tristate "Samsung S3C64XX series type SPI" depends on PLAT_SAMSUNG && DMADEVICES depends on S3C64XX_PL080 if ARCH_S3C64XX The driver will cleanly fail if it can't get the DMA channels that it requires to operate, so we don't need to do anything more than this here. > > > drivers/spi/Kconfig:429: symbol SPI_S3C64XX depends on SPI > > > drivers/spi/Kconfig:8: symbol SPI is selected by DRM_PANEL_LD9040 > > 'select SPI' is also really bad. This seems trivial to replace with > 'depends on SPI'. This is the only driver that uses select here. Definitely. > > > drivers/gpu/drm/panel/Kconfig:19: symbol DRM_PANEL_LD9040 depends on DRM_PANEL > > > drivers/gpu/drm/panel/Kconfig:1: symbol DRM_PANEL is selected by DRM_IMX_LDB > > > drivers/staging/imx-drm/Kconfig:35: symbol DRM_IMX_LDB depends on MFD_SYSCON > > > drivers/mfd/Kconfig:722: symbol MFD_SYSCON is selected by POWER_RESET_KEYSTONE > > We have 10 drivers doing 'select MFD_SYSCON' and 9 drivers doing > 'depends on MFD_SYSCON'. Either one would work if we did those > consistently, here the problem is really mixing the two. Indeed - we need clear policies on this stuff, because as we get different platforms doing different things (as is the case in this scenario), we are only going to see an increase in this problem. Also, I don't think we should do just one change to break this loop - this is a sign of a much bigger disease. We are heading towards the situation where adding just one 'select' or 'depends on' statement between two symbols, thereby adding just one more dependency to an already tightly linked graph can cause a circular dependency. We need to stop this behaviour ASAP, and kill off as many of these ill-considered or wrong dependencies as possible, otherwise we're risking Kconfig becoming unmaintainable. Whenever we see a new 'select' statment appearing in a patch for something which is not a utility symbol, we need to ask what the justification is for it being there, and evaluate whether it's reasonable (ideally, this should be detailed in the commit log.) (I define a utility symbol as one whose primary purpose is to be selected.) -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Kconfig fails: big select-based circular dependency 2014-06-12 11:34 ` Russell King - ARM Linux @ 2014-06-12 11:57 ` Geert Uytterhoeven 2014-06-12 12:18 ` Mark Brown 2014-06-12 12:19 ` Arnd Bergmann 1 sibling, 1 reply; 13+ messages in thread From: Geert Uytterhoeven @ 2014-06-12 11:57 UTC (permalink / raw) To: linux-arm-kernel On Thu, Jun 12, 2014 at 1:34 PM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: >> > > drivers/gpu/drm/panel/Kconfig:19: symbol DRM_PANEL_LD9040 depends on DRM_PANEL >> > > drivers/gpu/drm/panel/Kconfig:1: symbol DRM_PANEL is selected by DRM_IMX_LDB >> > > drivers/staging/imx-drm/Kconfig:35: symbol DRM_IMX_LDB depends on MFD_SYSCON >> > > drivers/mfd/Kconfig:722: symbol MFD_SYSCON is selected by POWER_RESET_KEYSTONE >> >> We have 10 drivers doing 'select MFD_SYSCON' and 9 drivers doing >> 'depends on MFD_SYSCON'. Either one would work if we did those >> consistently, here the problem is really mixing the two. > > Indeed - we need clear policies on this stuff, because as we get different > platforms doing different things (as is the case in this scenario), we are > only going to see an increase in this problem. > > Also, I don't think we should do just one change to break this loop - this > is a sign of a much bigger disease. We are heading towards the situation > where adding just one 'select' or 'depends on' statement between two symbols, > thereby adding just one more dependency to an already tightly linked graph > can cause a circular dependency. > > We need to stop this behaviour ASAP, and kill off as many of these > ill-considered or wrong dependencies as possible, otherwise we're risking > Kconfig becoming unmaintainable. > > Whenever we see a new 'select' statment appearing in a patch for something > which is not a utility symbol, we need to ask what the justification is > for it being there, and evaluate whether it's reasonable (ideally, this > should be detailed in the commit log.) > > (I define a utility symbol as one whose primary purpose is to be selected.) "primary purpose". Should we have a better separation between select and depends, i.e. should kconf plainly reject both being used on the same symbol? Is it ever valid to have a symbol that's both selected and used with depends on? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 13+ messages in thread
* Kconfig fails: big select-based circular dependency 2014-06-12 11:57 ` Geert Uytterhoeven @ 2014-06-12 12:18 ` Mark Brown 2014-06-12 12:56 ` Paul Bolle 0 siblings, 1 reply; 13+ messages in thread From: Mark Brown @ 2014-06-12 12:18 UTC (permalink / raw) To: linux-arm-kernel On Thu, Jun 12, 2014 at 01:57:03PM +0200, Geert Uytterhoeven wrote: > Should we have a better separation between select and depends, i.e. > should kconf plainly reject both being used on the same symbol? > Is it ever valid to have a symbol that's both selected and used with depends on? Architecture feature Kconfig symbols typically have the pattern that the architecture selects ARCH_HAS_FOO and then code that needs the feature depends on ARCH_HAS_FOO. -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140612/87c4478b/attachment.sig> ^ permalink raw reply [flat|nested] 13+ messages in thread
* Kconfig fails: big select-based circular dependency 2014-06-12 12:18 ` Mark Brown @ 2014-06-12 12:56 ` Paul Bolle 0 siblings, 0 replies; 13+ messages in thread From: Paul Bolle @ 2014-06-12 12:56 UTC (permalink / raw) To: linux-arm-kernel On Thu, 2014-06-12 at 13:18 +0100, Mark Brown wrote: > Architecture feature Kconfig symbols typically have the pattern that the > architecture selects ARCH_HAS_FOO and then code that needs the feature > depends on ARCH_HAS_FOO. A similar usage can be seen with Kconfig symbols with a HAVE_BAR name. Paul Bolle ^ permalink raw reply [flat|nested] 13+ messages in thread
* Kconfig fails: big select-based circular dependency 2014-06-12 11:34 ` Russell King - ARM Linux 2014-06-12 11:57 ` Geert Uytterhoeven @ 2014-06-12 12:19 ` Arnd Bergmann 2014-06-12 13:03 ` Russell King - ARM Linux 1 sibling, 1 reply; 13+ messages in thread From: Arnd Bergmann @ 2014-06-12 12:19 UTC (permalink / raw) To: linux-arm-kernel On Thursday 12 June 2014 12:34:45 Russell King - ARM Linux wrote: > On Thu, Jun 12, 2014 at 12:31:19PM +0200, Arnd Bergmann wrote: > > On Thursday 12 June 2014 10:47:45 Russell King - ARM Linux wrote: > > > If no one responds, I'll assume that no one is interested, and I'll > > > just create a pile of patches removing a bunch of these idiotic select > > > statements at random to break this loop. > > > > I missed the original mail, and I don't remember seeing this particular > > dependency chain. > > > > > On Sat, Jun 07, 2014 at 10:09:44AM +0100, Russell King - ARM Linux wrote: > > > > This is getting silly: > > > > > > > > scripts/kconfig/conf --silentoldconfig Kconfig > > > > drivers/dma/Kconfig:5:error: recursive dependency detected! > > > > drivers/dma/Kconfig:5: symbol DMADEVICES is selected by SAMSUNG_DMADEV > > > > The 'select DMADEVICES' from plat-samsung/Kconfig is certainly wrong, > > we shouldn't do that, but I'd do some extended build regression tests > > to ensure that it doesn't cause other problems. > > > > I'll have a look. > > Yes, SAMSUNG_DMADEV looks like it's a shim layer between DMA engine and > the old Samsung platform private DMA API. I suspect SAMSUNG_DMADEV should > be selected by the drivers which make use of this shim iff DMADEVICES is > enabled. FWIW, SAMSUNG_DMADEV should get removed in 3.17. At the moment there is only one user (sound/soc/samsung/ac97.c), and that is broken because it calls a NULL function pointer returned from samsung_dma_get_ops(). > > > > arch/arm/plat-samsung/Kconfig:412: symbol SAMSUNG_DMADEV is selected by S3C64XX_PL080 > > I think killing the select statement against S3C64XX_PL080 of this shim is > also a correct move - I suspect that's just there to make Kconfig life > easier (which is not really a valid reason for adding a select statement > to a major subsystem.) > > > > > arch/arm/mach-s3c64xx/Kconfig:20: symbol S3C64XX_PL080 is selected by SPI_S3C64XX > > As I said in my reply to Paul, this select should also be killed. At > the very least, it should be turned into "depends on S3C64XX_PL080 if > ARCH_S3C64XX". > > Since the comments in the driver say that the driver depends on having a > DMA engine, I'd suggest also adding a dependency on DMADEVICES as well, > to encode that explicit requirement in the Kconfig language: > > config SPI_S3C64XX > tristate "Samsung S3C64XX series type SPI" > depends on PLAT_SAMSUNG && DMADEVICES > depends on S3C64XX_PL080 if ARCH_S3C64XX > > The driver will cleanly fail if it can't get the DMA channels that it > requires to operate, so we don't need to do anything more than this here. I have struggled with this dependency a few times before, the conversion from the samsung specific DMA driver to dmaengine did not go well, and we still see the fallout from that here. It seems this particular case was just a mistake that Tomasz made in 3faecea70b0 "spi: s3c64xx: Always select S3C64XX_PL080 when ARCH_S3C64XX is enabled" when the correct conversion would have been to drop the dependency. However, the arch/arm/plat-samsung/dma-ops.c code relies on either S3C64XX_PL080 or PL330_DMA (but not both!) to be enabled, and the dependency in the SPI driver happens to ensure that at the moment. When we remove it here, we have to change the plat-samsung code to either do the select there or fix it properly. I'm inclined to go for a hack here, because this code is going away anyway. diff --git a/arch/arm/plat-samsung/Kconfig b/arch/arm/plat-samsung/Kconfig index 8a22df5..704dbc6 100644 --- a/arch/arm/plat-samsung/Kconfig +++ b/arch/arm/plat-samsung/Kconfig @@ -416,6 +416,7 @@ config SAMSUNG_DMADEV select DMADEVICES select PL330_DMA if (ARCH_EXYNOS5 || ARCH_EXYNOS4 || CPU_S5PV210 || CPU_S5PC100 || \ CPU_S5P6450 || CPU_S5P6440) + select S3C64XX_PL080 if ARCH_S3C64XX help Use DMA device engine for PL330 DMAC. diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig index 213b5cb..33d935c 100644 --- a/drivers/spi/Kconfig +++ b/drivers/spi/Kconfig @@ -422,7 +422,6 @@ config SPI_S3C24XX_FIQ config SPI_S3C64XX tristate "Samsung S3C64XX series type SPI" depends on PLAT_SAMSUNG - select S3C64XX_PL080 if ARCH_S3C64XX help SPI driver for Samsung S3C64XX and newer SoCs. diff --git a/arch/arm/mach-s3c64xx/Kconfig b/arch/arm/mach-s3c64xx/Kconfig index d863722..8c1fe01 100644 --- a/arch/arm/mach-s3c64xx/Kconfig +++ b/arch/arm/mach-s3c64xx/Kconfig @@ -20,7 +20,6 @@ config CPU_S3C6410 config S3C64XX_PL080 bool "S3C64XX DMA using generic PL08x driver" select AMBA_PL08X - select SAMSUNG_DMADEV config S3C64XX_SETUP_SDHCI bool This would also avoid changing the defconfigs. > > > > drivers/gpu/drm/panel/Kconfig:19: symbol DRM_PANEL_LD9040 depends on DRM_PANEL > > > > drivers/gpu/drm/panel/Kconfig:1: symbol DRM_PANEL is selected by DRM_IMX_LDB > > > > drivers/staging/imx-drm/Kconfig:35: symbol DRM_IMX_LDB depends on MFD_SYSCON > > > > drivers/mfd/Kconfig:722: symbol MFD_SYSCON is selected by POWER_RESET_KEYSTONE > > > > We have 10 drivers doing 'select MFD_SYSCON' and 9 drivers doing > > 'depends on MFD_SYSCON'. Either one would work if we did those > > consistently, here the problem is really mixing the two. > > Indeed - we need clear policies on this stuff, because as we get different > platforms doing different things (as is the case in this scenario), we are > only going to see an increase in this problem. > > Also, I don't think we should do just one change to break this loop - this > is a sign of a much bigger disease. We are heading towards the situation > where adding just one 'select' or 'depends on' statement between two symbols, > thereby adding just one more dependency to an already tightly linked graph > can cause a circular dependency. > > We need to stop this behaviour ASAP, and kill off as many of these > ill-considered or wrong dependencies as possible, otherwise we're risking > Kconfig becoming unmaintainable. > > Whenever we see a new 'select' statment appearing in a patch for something > which is not a utility symbol, we need to ask what the justification is > for it being there, and evaluate whether it's reasonable (ideally, this > should be detailed in the commit log.) > > (I define a utility symbol as one whose primary purpose is to be selected.) Agreed. Ideally a symbol that gets selected by another one would always be a 'silent' one, i.e. not also be user selectable, but we can't really enforce that with thousands of symbols not following that. One common scenario seems valid, too: A platform selects a driver or subsystem, and the platform specific driver has a dependency on that. Things just get this messy if the driver itself does the select. Arnd ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Kconfig fails: big select-based circular dependency 2014-06-12 12:19 ` Arnd Bergmann @ 2014-06-12 13:03 ` Russell King - ARM Linux 2014-06-12 13:37 ` Arnd Bergmann 0 siblings, 1 reply; 13+ messages in thread From: Russell King - ARM Linux @ 2014-06-12 13:03 UTC (permalink / raw) To: linux-arm-kernel On Thu, Jun 12, 2014 at 02:19:45PM +0200, Arnd Bergmann wrote: > On Thursday 12 June 2014 12:34:45 Russell King - ARM Linux wrote: > > On Thu, Jun 12, 2014 at 12:31:19PM +0200, Arnd Bergmann wrote: > > > On Thursday 12 June 2014 10:47:45 Russell King - ARM Linux wrote: > > > > If no one responds, I'll assume that no one is interested, and I'll > > > > just create a pile of patches removing a bunch of these idiotic select > > > > statements at random to break this loop. > > > > > > I missed the original mail, and I don't remember seeing this particular > > > dependency chain. > > > > > > > On Sat, Jun 07, 2014 at 10:09:44AM +0100, Russell King - ARM Linux wrote: > > > > > This is getting silly: > > > > > > > > > > scripts/kconfig/conf --silentoldconfig Kconfig > > > > > drivers/dma/Kconfig:5:error: recursive dependency detected! > > > > > drivers/dma/Kconfig:5: symbol DMADEVICES is selected by SAMSUNG_DMADEV > > > > > > The 'select DMADEVICES' from plat-samsung/Kconfig is certainly wrong, > > > we shouldn't do that, but I'd do some extended build regression tests > > > to ensure that it doesn't cause other problems. > > > > > > I'll have a look. > > > > Yes, SAMSUNG_DMADEV looks like it's a shim layer between DMA engine and > > the old Samsung platform private DMA API. I suspect SAMSUNG_DMADEV should > > be selected by the drivers which make use of this shim iff DMADEVICES is > > enabled. > > FWIW, SAMSUNG_DMADEV should get removed in 3.17. At the moment > there is only one user (sound/soc/samsung/ac97.c), and that is > broken because it calls a NULL function pointer returned from > samsung_dma_get_ops(). Well, we can't wait for 3.17 to fix this, because attempting to build an allyesconfig/allmodconfig/randconfig today fails. Just look at the failures of those four configurations on my autobuilder over the last few days. In some cases, kconf generates a configuration, spitting out these warnings, but then when you try and build that configuration, it then goes on to complain that some symbols need user input. In other words, the first resulting configuration file from the make allyesconfig/ allmodconfig/randconfig is invalid because the dependencies have not reached stability. So, I suspect that there's more problems in Kconfig caused by this select madness, and I think we need a positive effort on sorting this crap out. If we let it continue, it will eventually get noticed on x86, and we will then have Linus flaming the ARM folk yet again... -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Kconfig fails: big select-based circular dependency 2014-06-12 13:03 ` Russell King - ARM Linux @ 2014-06-12 13:37 ` Arnd Bergmann 2014-06-12 14:17 ` Russell King - ARM Linux 0 siblings, 1 reply; 13+ messages in thread From: Arnd Bergmann @ 2014-06-12 13:37 UTC (permalink / raw) To: linux-arm-kernel On Thursday 12 June 2014 14:03:30 Russell King - ARM Linux wrote: > On Thu, Jun 12, 2014 at 02:19:45PM +0200, Arnd Bergmann wrote: > > On Thursday 12 June 2014 12:34:45 Russell King - ARM Linux wrote: > > > On Thu, Jun 12, 2014 at 12:31:19PM +0200, Arnd Bergmann wrote: > > > > On Thursday 12 June 2014 10:47:45 Russell King - ARM Linux wrote: > > > > > If no one responds, I'll assume that no one is interested, and I'll > > > > > just create a pile of patches removing a bunch of these idiotic select > > > > > statements at random to break this loop. > > > > > > > > I missed the original mail, and I don't remember seeing this particular > > > > dependency chain. > > > > > > > > > On Sat, Jun 07, 2014 at 10:09:44AM +0100, Russell King - ARM Linux wrote: > > > > > > This is getting silly: > > > > > > > > > > > > scripts/kconfig/conf --silentoldconfig Kconfig > > > > > > drivers/dma/Kconfig:5:error: recursive dependency detected! > > > > > > drivers/dma/Kconfig:5: symbol DMADEVICES is selected by SAMSUNG_DMADEV > > > > > > > > The 'select DMADEVICES' from plat-samsung/Kconfig is certainly wrong, > > > > we shouldn't do that, but I'd do some extended build regression tests > > > > to ensure that it doesn't cause other problems. > > > > > > > > I'll have a look. > > > > > > Yes, SAMSUNG_DMADEV looks like it's a shim layer between DMA engine and > > > the old Samsung platform private DMA API. I suspect SAMSUNG_DMADEV should > > > be selected by the drivers which make use of this shim iff DMADEVICES is > > > enabled. > > > > FWIW, SAMSUNG_DMADEV should get removed in 3.17. At the moment > > there is only one user (sound/soc/samsung/ac97.c), and that is > > broken because it calls a NULL function pointer returned from > > samsung_dma_get_ops(). > > Well, we can't wait for 3.17 to fix this, because attempting to build > an allyesconfig/allmodconfig/randconfig today fails. Just look at the > failures of those four configurations on my autobuilder over the last > few days. I didn't say we should not fix it, I just meant we don't need to spend too much time on a perfect solution for code that is going away and that is already not used anywhere. I've just replied to an older thread "Re: [PATCH 1/2] [RFC] ASoC: samsung: move s3c24xx over to dmaengine" with a patch that would let us kill off the code right away, or at least disable it in Kconfig. For some reason, I can't reproduce the failure you see in your build system, I tried torvalds/master and next/master today and I have also done allmodconfig and randconfig builds in the past few days on slightly older versions. > In some cases, kconf generates a configuration, spitting out these > warnings, but then when you try and build that configuration, it then > goes on to complain that some symbols need user input. In other words, > the first resulting configuration file from the make allyesconfig/ > allmodconfig/randconfig is invalid because the dependencies have not > reached stability. I think it just gives up when it sees a recursive dependency, so instead you start out with whatever the oldconfig was, rather than actually building the 'allmodconfig' you asked for. Arnd ^ permalink raw reply [flat|nested] 13+ messages in thread
* Kconfig fails: big select-based circular dependency 2014-06-12 13:37 ` Arnd Bergmann @ 2014-06-12 14:17 ` Russell King - ARM Linux 0 siblings, 0 replies; 13+ messages in thread From: Russell King - ARM Linux @ 2014-06-12 14:17 UTC (permalink / raw) To: linux-arm-kernel On Thu, Jun 12, 2014 at 03:37:18PM +0200, Arnd Bergmann wrote: > I didn't say we should not fix it, I just meant we don't need to spend > too much time on a perfect solution for code that is going away and that > is already not used anywhere. > > I've just replied to an older thread "Re: [PATCH 1/2] [RFC] ASoC: samsung: > move s3c24xx over to dmaengine" with a patch that would let us kill off > the code right away, or at least disable it in Kconfig. > > For some reason, I can't reproduce the failure you see in your build system, > I tried torvalds/master and next/master today and I have also done > allmodconfig and randconfig builds in the past few days on slightly > older versions. That's because you don't have this commit: Author: Philipp Zabel <p.zabel@pengutronix.de> Date: Thu Mar 6 14:54:39 2014 +0100 imx-drm: imx-ldb: add drm_panel support This patch allows to optionally attach the lvds-channel to a panel supported by a drm_panel driver instead of supplying the modes via device tree. Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> [Fixed build error due to missing select on DRM_PANEL --rmk] Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> diff --git a/drivers/staging/imx-drm/Kconfig b/drivers/staging/imx-drm/Kconfig index c6e8ba7b3e4e..92fb52cbd3a2 100644 --- a/drivers/staging/imx-drm/Kconfig +++ b/drivers/staging/imx-drm/Kconfig @@ -35,6 +35,7 @@ config DRM_IMX_TVE config DRM_IMX_LDB tristate "Support for LVDS displays" depends on DRM_IMX && MFD_SYSCON + select DRM_PANEL help Choose this to enable the internal LVDS Display Bridge (LDB) found on i.MX53 and i.MX6 processors. which introduces the final nail in the loop - and as imx-ldb really does require DRM_PANEL support, and DRM_PANEL is not a user selectable symbol, the above addition is an entirely reasonable thing to do. The above commit /was/ going to go in during this merge window (along with three others) had I not been soo hacked off with the crap change handling and the general dysfunctional ARM community that we seem to have in the kernel community now, and ended up walking totally away from kernel maintanence for much of the last cycle... we're only /just/ starting to find out all the problems that my MMC patch series caused... and there's /still/ outstanding issues with the L2C patch series which /still/ have not been resolved - both of which are now part of mainline so people will now be forced to deal with these issues. That's not the right way, but it seems to be the /only/ way to get things done in todays dysfunctional environment. -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it. ^ permalink raw reply related [flat|nested] 13+ messages in thread
end of thread, other threads:[~2014-06-12 14:17 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-06-07 9:09 Kconfig fails: big select-based circular dependency Russell King - ARM Linux 2014-06-12 9:47 ` Russell King - ARM Linux 2014-06-12 10:22 ` Paul Bolle 2014-06-12 10:49 ` Russell King - ARM Linux 2014-06-12 10:31 ` Arnd Bergmann 2014-06-12 11:34 ` Russell King - ARM Linux 2014-06-12 11:57 ` Geert Uytterhoeven 2014-06-12 12:18 ` Mark Brown 2014-06-12 12:56 ` Paul Bolle 2014-06-12 12:19 ` Arnd Bergmann 2014-06-12 13:03 ` Russell King - ARM Linux 2014-06-12 13:37 ` Arnd Bergmann 2014-06-12 14:17 ` Russell King - ARM Linux
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).