Message-ID: <4d5c9bfc-dc10-7019-cad4-751e21f02a18@huawei.com>
Date:   Mon, 25 Jul 2022 15:03:23 +0100
From:   John Garry <john.garry@...wei.com>
To:     Arnd Bergmann <arnd@...db.de>
CC:     Will Deacon <will@...nel.org>,
        Catalin Marinas <catalin.marinas@....com>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        Olof Johansson <olof@...om.net>, SoC Team <soc@...nel.org>,
        <jpoimboe@...nel.org>,
        "Linux Kernel Mailing List" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 0/2] arm64 defconfig: Get faddr2line working

On 25/07/2022 13:51, Arnd Bergmann wrote:
>>   From commit fdf4632aa834, we enable config ATH11K_PCI, which selects

That should have been 231a136fdf4632a - I chopped off the leading 
characters by accident.

>> config QRTR, so no need to explicitly enable in the defconfig.
> Something is wrong here, as qrtr is used the same way in both ath10k and
> ath11k, but only one of them selects the symbol. My guess is that there is
> no actual hard requirement to enable it.

I'm not sure. From commit 1399fb87ea3e ("ath11k: register MHI controller 
device for QCA6390"), QRTR/MHI bus seems to be used for ath11k only:

Extract from that commit:

diff --git a/drivers/net/wireless/ath/ath11k/Kconfig 
b/drivers/net/wireless/ath/ath11k/Kconfig
index 2a792ddd6fea..7e5094e0e7bb 100644
--- a/drivers/net/wireless/ath/ath11k/Kconfig
+++ b/drivers/net/wireless/ath/ath11k/Kconfig
@@ -21,6 +21,9 @@ config ATH11K_AHB
config ATH11K_PCI
        tristate "Atheros ath11k PCI support"
        depends on ATH11K && PCI
+       select MHI_BUS
+       select QRTR
+       select QRTR_MHI


> 
>>> CONFIG_PINCTRL_MSM=y
>>   From commit c807a335d3b1, config PINCTRL_SM8450 is enabled, which
>> selects PINCTRL_MSM, so no need to explicitly enable in the defconfig.
> This looks like a bug, where the driver should have used 'depends on'
> rather than 'select'.

ok, I see that this is the odd one out in that kconfig as the other 
configs "depends on", like you say

> 
>>> CONFIG_SND_SOC_TEGRA210_OPE=m
>> There is no issue on next-20220722.
>>
>> On arm-soc arm/defconfig, config SND_SOC_TEGRA210_OPE just does not yet
>> exist, so that's why it get removed from the sync of the defconfig.
> Ok
> 
>>> CONFIG_MAILBOX=y
>> In commit fc739069aa92, config MAILBOX was enabled in the defconfig but
>> it was already being enabled from elsewhere. There was definitely no
>> sync of the savedefconfig going on there:)
> I see it's selected by a couple of drivers using mailboxes, and I
> agree we shouldn't
> need it here. It might be good to just hide the symbol in this case
> and select it
> consistently from all drivers using it.

Uhh, we have ~15x "select" and ~18x "depends on"...

> 
>>> CONFIG_QCOM_ICC_BWMON=m
>> Commit 76f11e77f919 enabled config QCOM_ICC_BWMON, but like config
>> ARCH_BCMBCA, that config does not exist on arm-soc arm/defconfig branch
>>
>> On next-20220722, no sync is required
> ok
> 
>>> CONFIG_SLIMBUS=m
>> Config 5bd773242f75 added a kconfig "imply" on config SLIMBUS from
>> config SOUNDWIRE_QCOM, and this happens to mean that we don't explicitly
>> require config SLIMBUS enabled in the defconfig.
> That 'imply' looks like it was added to solve a problem using the old 'imply'
> semantics that are not what this keyword does today.

I didn't even know it existed.

> I suspect it's still
> broken when CONFIG=SOUNDWIRE_QCOM=y and CONFIG_SLIBMUS=m,
> and the correct fix is to use a dependency like
> 
>        depends on SLIMBUS || !SLIMBUS
> 
> so the defconfig symbol should stay.

Let me see if I can break it and then fix it.

> 
>>> CONFIG_INTERCONNECT=y
>> Since commit 06f079816d4c, config TEGRA_MC added a kconfig select on
>> config INTERCONNECT, so no need to explicitly enable from the kconfig
> We have one driver using 'depends on INTERCONNECT' and two drivers
> using 'select INTERCONNECT', so at least one of them is wrong.

INTERCONNECT has no dependencies, so "select" - like for MAILBOX - 
should be fine, I suppose

> 
> There are also a couple of drivers that use neither and instead rely on the
> IS_ENABLED() check in include/linux/interconnect.h, but this is potentially
> wrong in the same way as the SLIMBUS dependency above.
> 
> This clearly needs some more discussion. I would suggest removing the
> #iff check in the header and forcing all users to use 'depends on', leaving
> the defconfig symbol.
> 
>>> CONFIG_CONFIGFS_FS=y
>>   From commit cd8bc7d4eb66, config PCI_ENDPOINT_CONFIGFS is enabled in
>> the defconfig, and this selects CONFIG_CONFIGFS_FS, so no need to have
>> explicit enabling in the defconfig.
> Again, half the users of CONFIGFS_FS use 'depends on', the other half use
> 'select'. We should be consistent here and always use the same method,
> probably 'depends on' if we want to keep it as user-visible.

I don't know ....

> 
>> We still have issues on next-22072022:
>> CONFIG_ARM_CPUIDE
>> CONFIG_DEBUG_INFO
>> CONFIG_DEBUG_INFO_REDUCED
>>
>> The latter two are not an issue on the arm-soc arm/defconfig.
> Ok.
> 
>> So let me know if any comments or how to proceed.
>>
>> And would each config item deletion merit a separate patch?
> You send a combined patch for the obvious ones (secccomp
> and mailbox AFAICT) or send them separately. For the other ones I think
> we should try fixing the Kconfig files first, otherwise we just end up
> putting them back afterwards.

ok, fine. I'll deal with the obvious changes first plus 
CONFIG_DEBUG_INFO and then the non-obvious, non-trivial ones. I'll base 
on your arm/defconfig branch (for defconfig changes).

Thanks,
John

Powered by blists - more mailing lists