On Wed, 22 Oct 2014, Tomi Valkeinen tomi.valkeinen@ti.com wrote:
On 18/10/14 00:13, Jani Nikula wrote:
Documentation/kbuild/kconfig-language.txt warns to use select with care, and in general use select only for non-visible symbols and for symbols with no dependencies, because select will force a symbol to a value without visiting the dependencies.
Select has become particularly problematic, interdependently, with the BACKLIGHT_CLASS_DEVICE and ACPI_VIDEO configs. For example:
scripts/kconfig/conf --randconfig Kconfig KCONFIG_SEED=0x48312B00 warning: (DRM_RADEON && DRM_NOUVEAU && DRM_I915 && DRM_GMA500 && DRM_SHMOBILE && DRM_TILCDC && FB_BACKLIGHT && FB_MX3 && USB_APPLEDISPLAY && FB_OLPC_DCON && ASUS_LAPTOP && SONY_LAPTOP && THINKPAD_ACPI && EEEPC_LAPTOP && ACPI_CMPC && SAMSUNG_Q10) selects BACKLIGHT_CLASS_DEVICE which has unmet direct dependencies (HAS_IOMEM && BACKLIGHT_LCD_SUPPORT) warning: (DRM_RADEON && DRM_NOUVEAU && DRM_I915 && DRM_GMA500 && DRM_SHMOBILE && DRM_TILCDC && FB_BACKLIGHT && FB_MX3 && USB_APPLEDISPLAY && FB_OLPC_DCON && ASUS_LAPTOP && SONY_LAPTOP && THINKPAD_ACPI && EEEPC_LAPTOP && ACPI_CMPC && SAMSUNG_Q10) selects BACKLIGHT_CLASS_DEVICE which has unmet direct dependencies (HAS_IOMEM && BACKLIGHT_LCD_SUPPORT)
With tristates it's possible to end up selecting FOO=y depending on BAR=m in the config, which gets discovered at build time, not config time, like reported in the thread referenced below.
Do the following to fix the dependencies:
Depend on instead of select BACKLIGHT_CLASS_DEVICE everywhere. Drop select BACKLIGHT_LCD_SUPPORT in such cases, as it's a dependency of BACKLIGHT_CLASS_DEVICE.
Remove config FB_BACKLIGHT altogether, and replace it with a dependency on BACKLIGHT_CLASS_DEVICE. All configs selecting FB_BACKLIGHT select or depend on FB anyway, so we can simplify.
Depend on (ACPI && ACPI_VIDEO) || ACPI=n in several places instead of selecting ACPI_VIDEO and a number of its dependencies if ACPI is enabled. This is tied to backlight, as ACPI_VIDEO depends on BACKLIGHT_CLASS_DEVICE.
Replace a couple of select INPUT/VT with depends as it seemed to be necessary.
While doing 'depends on' instead of 'select' is an "easy" fix for this, I do dislike it quite a bit. It's a major pain to go around the kernel config, trying to find all the dependencies that a particular driver wants. If I need fb-foobar, I should just be able to enable it, instead of first searching and selecting its minor dependencies individually.
Agreed, but I don't think that's specific to this patch.
So, not a NACK, but a "isn't there an another way to fix this?".
I think the real answer would be to fix kconfig to also show menu items whose dependencies are not met, and then recursively enabling the dependencies when the item is enabled. Beyond my scope.
Looking at backlight... BACKLIGHT_LCD_SUPPORT seems to be a "meta" option, it only enables a Kconfig submenu.
So I think we could just remove the whole BACKLIGHT_LCD_SUPPORT option. But if we do that, all the items in drivers/video/backlight/Kconfig with default 'y' or 'm' would get enabled by default, so I think we should remove the 'default's from that file. That makes sense in any case, as I don't see why "HP Jornada 700 series LCD Driver" should be "default y".
BACKLIGHT_CLASS_DEVICE doesn't depend on anything except BACKLIGHT_LCD_SUPPORT, so after removing BACKLIGHT_LCD_SUPPORT it should be safe to 'select' BACKLIGHT_CLASS_DEVICE.
BACKLIGHT_CLASS_DEVICE could be made a hidden option, and the drivers in drivers/video/backlight/Kconfig which are under BACKLIGHT_CLASS_DEVICE could be made to select BACKLIGHT_CLASS_DEVICE instead.
I think it should be possible to choose between y and m when it's selected, and it should be possible to enable it when it's not selected by any drivers. I'm not sure a hidden option is good for that.
That doesn't exactly fix anything, but I think it makes sense as BACKLIGHT_CLASS_DEVICE is something that's selected from all around the kernel, so it should be a selectable "library" instead of a Kconfig menu option.
At least for drm/i915 BACKLIGHT_CLASS_DEVICE is "an option". We use it if it's enabled, but we are just fine if it's not. I've learned the way to express that is
depends on BACKLIGHT_CLASS_DEVICE || BACKLIGHT_CLASS_DEVICE=n
but I don't think there's a way to express that in terms of select, is there? The dependency above guarantees there's no DRM_I915=y and BACKLIGHT_CLASS_DEVICE=m combo which would fail. And this, btw, is where this whole patch got started, as select didn't handle that properly.
I didn't look at the ACPI_VIDEO side, so no idea how messy that is.
Basically it's another dependency on BACKLIGHT_CLASS_DEVICE. I can only imagine trying to solve this problem with select is going to end up in recursive dependencies that spread out and need changing about as wide as this patch.
In the end, I agree with the problem you have with this patch, but yet I think it's the right thing to do in terms of expressing the dependencies.
BR, Jani.