linux-kernel - Re: [PATCH 1/2] fbdev: don't select I2C directly

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Thu, 1 Feb 2018 16:21:08 -0800
From:   Randy Dunlap <rdunlap@...radead.org>
To:     Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>,
        Arnd Bergmann <arnd@...db.de>
Cc:     Rob Clark <robdclark@...il.com>,
        Jordan Crouse <jcrouse@...eaurora.org>,
        dri-devel@...ts.freedesktop.org, linux-fbdev@...r.kernel.org,
        linux-i2c@...r.kernel.org, Wolfram Sang <wsa@...-dreams.de>,
        Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] fbdev: don't select I2C directly

On 02/01/2018 08:14 AM, Bartlomiej Zolnierkiewicz wrote:
> On Monday, January 15, 2018 05:14:04 PM Arnd Bergmann wrote:
>> Using a Kconfig 'select' statement for a user-visible symbol that other
>> drivers depend on often causes circular dependencies. A new one showed
>> up when I wanted to add an NVMEM dependency to the DRM_MSM driver:
>>
>> drivers/i2c/Kconfig:7:error: recursive dependency detected!
>> drivers/i2c/Kconfig:7:	symbol I2C is selected by FB_DDC
>> drivers/video/fbdev/Kconfig:63:	symbol FB_DDC is selected by FB_CYBER2000_DDC
>> drivers/video/fbdev/Kconfig:390:	symbol FB_CYBER2000_DDC depends on FB_CYBER2000
>> drivers/video/fbdev/Kconfig:378:	symbol FB_CYBER2000 depends on FB
>> drivers/video/fbdev/Kconfig:5:	symbol FB is selected by DRM_KMS_FB_HELPER
>> drivers/gpu/drm/Kconfig:77:	symbol DRM_KMS_FB_HELPER depends on DRM_KMS_HELPER
>> drivers/gpu/drm/Kconfig:71:	symbol DRM_KMS_HELPER is selected by DRM_MSM
>> drivers/gpu/drm/msm/Kconfig:2:	symbol DRM_MSM depends on NVMEM
>> drivers/nvmem/Kconfig:1:	symbol NVMEM is selected by EEPROM_AT24
>> drivers/misc/eeprom/Kconfig:3:	symbol EEPROM_AT24 depends on I2C
>>
>> Here, the problem is that many fbdev drivers have an i2c based interface
>> and just 'select i2c' for that, while we also select the framebuffer
>> subsystem indirectly from a DRM driver that now depends on i2c.
>>
>> This does away with the 'select' statement and instead uses 'depends on',
>> like almost all I2C users.
> 
> I worry that this change may cause various driver options to no longer
> be visible to people configuring their kernels and not having I2C already
> selected.
> 
> DRM somehow manages to select I2C and I would prefer to be it the same
> way for fbdev (also looking at the current next tree there are still
> some drivers that 'select I2C')..

a. Linus has stated that a driver should not enable an entire subsystem,
   so depends on would be better than select.
   If I had the email/patch, I would be glad to Ack it.

b. DRM configuration is a mess. You shouldn't want to follow their model. :)

c. If I2C is not enabled in the FB menu, someone could just add something like this:

comment "Enable I2C to see more driver choices"
	depends on !I2C


>> Signed-off-by: Arnd Bergmann <arnd@...db.de>
>> ---
>>  drivers/video/fbdev/Kconfig | 19 +++++++++++++++----
>>  1 file changed, 15 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig
>> index 6962b4583fd7..892eb1863100 100644
>> --- a/drivers/video/fbdev/Kconfig
>> +++ b/drivers/video/fbdev/Kconfig
>> @@ -64,7 +64,6 @@ config FB_DDC
>>         tristate
>>         depends on FB
>>         select I2C_ALGOBIT
>> -       select I2C
>>         default n
>>  
>>  config FB_BOOT_VESA_SUPPORT
>> @@ -390,6 +389,7 @@ config FB_CYBER2000
>>  config FB_CYBER2000_DDC
>>  	bool "DDC for CyberPro support"
>>  	depends on FB_CYBER2000
>> +	depends on I2C=y || I2C=FB_CYBER2000
>>  	select FB_DDC
>>  	default y
>>  	help
>> @@ -634,7 +634,7 @@ config FB_BF537_LQ035
>>  config FB_BFIN_7393
>>  	tristate "Blackfin ADV7393 Video encoder"
>>  	depends on FB && BLACKFIN
>> -	select I2C
>> +	depends on I2C
>>  	select FB_CFB_FILLRECT
>>  	select FB_CFB_COPYAREA
>>  	select FB_CFB_IMAGEBLIT
>> @@ -1026,6 +1026,7 @@ config FB_NVIDIA
>>  config FB_NVIDIA_I2C
>>         bool "Enable DDC Support"
>>         depends on FB_NVIDIA
>> +       depends on I2C=y || I2C=FB_NVIDIA
>>         select FB_DDC
>>         help
>>  	  This enables I2C support for nVidia Chipsets.  This is used
>> @@ -1073,6 +1074,7 @@ config FB_RIVA
>>  config FB_RIVA_I2C
>>         bool "Enable DDC Support"
>>         depends on FB_RIVA
>> +       depends on I2C=y || I2C=FB_RIVA
>>         select FB_DDC
>>         help
>>  	  This enables I2C support for nVidia Chipsets.  This is used
>> @@ -1102,6 +1104,7 @@ config FB_RIVA_BACKLIGHT
>>  config FB_I740
>>  	tristate "Intel740 support"
>>  	depends on FB && PCI
>> +	depends on I2C
>>  	select FB_MODE_HELPERS
>>  	select FB_CFB_FILLRECT
>>  	select FB_CFB_COPYAREA
>> @@ -1155,6 +1158,7 @@ config FB_I810_GTF
>>  config FB_I810_I2C
>>  	bool "Enable DDC Support"
>>  	depends on FB_I810 && FB_I810_GTF
>> +	depends on I2C=y || I2C=FB_I810
>>  	select FB_DDC
>>  	help
>>  
>> @@ -1206,6 +1210,7 @@ config FB_INTEL_DEBUG
>>  config FB_INTEL_I2C
>>  	bool "DDC/I2C for Intel framebuffer support"
>>  	depends on FB_INTEL
>> +	depends on I2C=y || I2C=FB_INTEL
>>  	select FB_DDC
>>  	default y
>>  	help
>> @@ -1285,6 +1290,7 @@ config FB_MATROX_G
>>  config FB_MATROX_I2C
>>  	tristate "Matrox I2C support"
>>  	depends on FB_MATROX
>> +	depends on I2C
>>  	select FB_DDC
>>  	---help---
>>  	  This drivers creates I2C buses which are needed for accessing the
>> @@ -1350,6 +1356,7 @@ config FB_RADEON
>>  config FB_RADEON_I2C
>>  	bool "DDC/I2C for ATI Radeon support"
>>  	depends on FB_RADEON
>> +	depends on I2C=y || I2C=FB_RADEON
>>  	select FB_DDC
>>  	default y
>>  	help
>> @@ -1460,6 +1467,7 @@ config FB_S3
>>  config FB_S3_DDC
>>  	bool "DDC for S3 support"
>>  	depends on FB_S3
>> +	depends on I2C=y || I2C=FB_S3
>>  	select FB_DDC
>>  	default y
>>  	help
>> @@ -1485,6 +1493,7 @@ config FB_SAVAGE
>>  config FB_SAVAGE_I2C
>>         bool "Enable DDC2 Support"
>>         depends on FB_SAVAGE
>> +       depends on I2C=y || I2C=FB_SAVAGE
>>         select FB_DDC
>>         help
>>  	  This enables I2C support for S3 Savage Chipsets.  This is used
>> @@ -1629,6 +1638,7 @@ config FB_3DFX_ACCEL
>>  config FB_3DFX_I2C
>>  	bool "Enable DDC/I2C support"
>>  	depends on FB_3DFX
>> +	depends on I2C=y || I2C=FB_3DFX
>>  	select FB_DDC
>>  	default y
>>  	help
>> @@ -1669,6 +1679,7 @@ config FB_VT8623
>>  config FB_TRIDENT
>>  	tristate "Trident/CyberXXX/CyberBlade support"
>>  	depends on FB && PCI
>> +	depends on I2C
>>  	select FB_CFB_FILLRECT
>>  	select FB_CFB_COPYAREA
>>  	select FB_CFB_IMAGEBLIT
>> @@ -2299,8 +2310,8 @@ endchoice
>>  
>>  config FB_MB862XX_I2C
>>  	bool "Support I2C bus on MB862XX GDC"
>> -	depends on FB_MB862XX && I2C
>> -	depends on FB_MB862XX=m || I2C=y
>> +	depends on FB_MB862XX
>> +	depends on I2C=y || I2C=FB_MB862XX
>>  	default y
>>  	help
>>  	  Selecting this option adds Coral-P(A)/Lime GDC I2C bus adapter


-- 
~Randy