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.
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
> [email protected]
> 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.
[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...
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
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.
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.
On Thu, Jun 12, 2014 at 1:34 PM, Russell King - ARM Linux
<[email protected]> 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 -- [email protected]
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
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.
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
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
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.
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
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 <[email protected]>
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 <[email protected]>
[Fixed build error due to missing select on DRM_PANEL --rmk]
Signed-off-by: Russell King <[email protected]>
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.