linux-kernel - Re: [PATCH 2/2] media: pci: mgb4: remove bogus 'select' statements

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:   Mon, 13 Nov 2023 15:57:01 +0100
From:   Hans Verkuil <hverkuil-cisco@...all.nl>
To:     Arnd Bergmann <arnd@...db.de>,
        Martin Tůma <tumic@...see.org>,
        Arnd Bergmann <arnd@...nel.org>,
        Martin Tuma <martin.tuma@...iteqautomotive.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>
Cc:     linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] media: pci: mgb4: remove bogus 'select' statements

On 10/11/2023 15:37, Arnd Bergmann wrote:
> On Wed, Nov 8, 2023, at 19:33, Martin Tůma wrote:
>> On 08. 11. 23 17:13, Arnd Bergmann wrote:
>>> On Fri, Oct 27, 2023, at 16:17, Martin Tůma wrote:
>>>> On 23. 10. 23 18:05, Arnd Bergmann wrote:
>>>>> From: Arnd Bergmann <arnd@...db.de>
>>
>> On SoCs you probably get a kernel configuration that is missing some 
>> feature but still boots up when you do not select/depend on the exact 
>> controller, but in the case of the mgb4 PCIe card you get a driver that 
>> does not work at all (The SPI_XILINX dependency could theoretically be 
>> made configurable, but you would lose the ability to flash the correct 
>> FW for the current HW module and the access to the card's serial number. 
>> I2C and XDMA are crucial.).
> 
> My point was that we do this all the time for things that are
> essential: if your clock controller or the irqchip have
> no driver, then the camera device won't work, but neither
> would anything else.
> 
> So in a SoC setting, you really just need to enable all
> the drivers for devices on that chip through the .config.
> 
>>> Since this is a PCI device, it's a bit different, so maybe
>>> something like this would work to correctly document which
>>> dependencies are required at build time vs run time:
>>>
>>> --- a/drivers/media/pci/mgb4/Kconfig
>>> +++ b/drivers/media/pci/mgb4/Kconfig
>>> @@ -1,15 +1,13 @@
>>>   # SPDX-License-Identifier: GPL-2.0-only
>>>   config VIDEO_MGB4
>>>          tristate "Digiteq Automotive MGB4 support"
>>> -       depends on VIDEO_DEV && PCI && I2C && DMADEVICES && SPI && MTD && IIO
>>> +       depends on VIDEO_DEV && PCI && I2C && SPI && MTD && IIO
>>>          depends on COMMON_CLK
>>> +       depends on XILINX_XDMA
>>> +       depends on (I2C_XILINX && SPI_XILINX) || COMPILE_TEST
>>>          select VIDEOBUF2_DMA_SG
>>>          select IIO_BUFFER
>>>          select IIO_TRIGGERED_BUFFER
>>> -       select I2C_XILINX
>>> -       select SPI_XILINX
>>> -       select MTD_SPI_NOR
>>> -       select XILINX_XDMA
>>>          help
>>>            This is a video4linux driver for Digiteq Automotive MGB4 grabber
>>>            cards.
>>>
>>
>> My motivation when using "select" was to help people using "make 
>> menuconfig" to get the module selected/configured as they will usually 
>> not know that there are some Xilinx IP cores used that need separate 
>> drivers and the menuconfig GUI simply hides the mgb4 option making it 
>> almost impossible just from the menus to find out what has to be selected.
>>
>> But when there are reasons, why to chose "depends on" (like various 
>> configurations, tests or the "readability" of the dependencies) than I'm 
>> ok with your patch proposal.
> 
> The main reason to use 'depends on' over 'select' here is that
> mixing the two is a common source of dependency loops that end
> up breaking the build. As a rule of thumb, I would use 'select'
> only for symbols that others already select, or that are hidden
> from visibility.
> 
>       Arnd
> 

Arnd, can you post a v2? I think this should go to v6.7 as a fix.

Regards,

	Hans