Re: [PATCH v2 05/11] arm64: defconfig: rebuild default configuration

[Krzysztof Kozlowski]

On 17/01/2022 13:05, Marcel Ziswiler wrote:
> On Sat, 2022-01-15 at 18:02 +0100, Krzysztof Kozlowski wrote:
>> On 14/01/2022 15:15, Marcel Ziswiler wrote:
>>> From: Marcel Ziswiler <marcel.ziswiler@xxxxxxxxxxx>
>>>
>>> Run "make defconfig; make savedefconfig" to rebuild defconfig.
>>>
>>> This re-ordered the following configuration options:
>>>
>>> CONFIG_BPF_JIT=y
>>> CONFIG_ARM_SCMI_PROTOCOL=y
>>> CONFIG_ARM_SCPI_PROTOCOL=y
>>> CONFIG_RASPBERRYPI_FIRMWARE=y
>>> CONFIG_INTEL_STRATIX10_SERVICE=y
>>> CONFIG_INTEL_STRATIX10_RSU=m
>>> CONFIG_EFI_CAPSULE_LOADER=y
>>> CONFIG_IMX_SCU=y
>>> CONFIG_IMX_SCU_PD=y
>>> CONFIG_CAN_FLEXCAN=m
>>> CONFIG_PCIE_LAYERSCAPE_GEN4=y
>>> CONFIG_MTK_DEVAPC=m
>>> CONFIG_SPI_CADENCE_QUADSPI=y
>>> CONFIG_MDIO_BUS_MUX_MMIOREG=y
>>> CONFIG_MDIO_BUS_MUX_MULTIPLEXER=y
>>> CONFIG_MESON_GXL_PHY=m
>>> CONFIG_QCOM_CPR=y
>>> CONFIG_ROCKCHIP_IODOMAIN=y
>>> CONFIG_SENSORS_ARM_SCMI=y
>>> CONFIG_QORIQ_THERMAL=m
>>> CONFIG_SUN8I_THERMAL=y
>>> CONFIG_TEGRA_BPMP_THERMAL=m
>>> CONFIG_ARM_SMC_WATCHDOG=y
>>> CONFIG_VIDEO_QCOM_CAMSS=m
>>> CONFIG_DRM_PANEL_BOE_TV101WUM_NL6=m
>>> CONFIG_DRM_NWL_MIPI_DSI=m
>>> CONFIG_DRM_LONTIUM_LT9611UXC=m
>>> CONFIG_SND_SOC_IMX_AUDMIX=m
>>> CONFIG_TYPEC_HD3SS3220=m
>>> CONFIG_COMMON_CLK_SCMI=y
>>> CONFIG_IPQ_GCC_8074=y
>>> CONFIG_SM_DISPCC_8250=y
>>> CONFIG_QCOM_WCNSS_CTRL=m
>>> CONFIG_ARCH_R8A774A1=y
>>> CONFIG_ARCH_R8A774B1=y
>>> CONFIG_ARCH_R8A774C0=y
>>> CONFIG_ARCH_R8A774E1=y
>>> CONFIG_ARCH_R8A77995=y
>>> CONFIG_ARCH_R8A77990=y
>>> CONFIG_ARCH_R8A77965=y
>>> CONFIG_ARCH_R8A77970=y
>>> CONFIG_HISI_PMU=y
>>> CONFIG_QCOM_QFPROM=y
>>> CONFIG_MUX_MMIO=y
>>
>> Thanks for the changes.
>> The best would be to have a separate patch only for re-ordering.
> 
> Yes, from a review perspective that makes sense. Will do so in v3.
> 
>>> And dropped the following configuration options which are nowaday's
>>> already enabled (resp. disabled) by default:
>>>
>>> CONFIG_MEMCG_SWAP=y
>>> CONFIG_SECCOMP=y
>>
>> Is it? I tried now on next-20220114 and it is still user-selectable and
>> not chosen by anything.
> 
> Hm, strange. I guess, it is due to this whole patch series, given its i.MX main focus, being based on Shawn's
> for-next branch. Maybe for such defconfig changes it would be better to base them on something else? Not sure,
> who will ultimately pull such changes. Any suggestion?

I don't think there is a one nice solution, fit-for-all.

Consider a defconfig selecting FOO and BAR, plus Kconfig like:
config FOO
	depends on BAR

If you run savedefconfig on arm-soc next branch, there is a chance that
in parallel someone else will adjust the Kconfig in some other tree,
based on Linus' master. That person removes "depends on BAR" above and
notices that FOO is mentioned in defconfig, so no need to update defconfig.

However your savedefconfig commit will exactly remove the FOO from
defconfig, because there is no reason for it to be. After merging to
Linus - FOO is gone from defconfig.

Another approach - you base your savedefconfig on linux-next. This
should solve the problem above, except that such commit might not apply
on the maintainer's tree.

Therefore recommendation is to not run savedefconfig or run it very
carefully, for example explaining why each option is being removed. I
was doing it in the past, but it's quite time consuming:

https://lore.kernel.org/linux-arm-kernel/1500270596-22384-1-git-send-email-krzk@xxxxxxxxxx/

and earlier set:
https://lore.kernel.org/linux-arm-kernel/20170608161049.12421-1-krzk@xxxxxxxxxx/

> 
>>> CONFIG_CPU_FREQ_GOV_SCHEDUTIL=y
>>> CONFIG_QCOM_SCM=y
>>> # CONFIG_BT_HS is not set
>>> CONFIG_FSL_MC_BUS=y
>>> CONFIG_MEDIA_CONTROLLER=y
>>> CONFIG_VIDEO_V4L2_SUBDEV_API=y
>>> CONFIG_SND_SOC_FSL_SAI=m
>>> CONFIG_USB_CONN_GPIO=m
>>> CONFIG_USB_XHCI_PCI=m
>>> CONFIG_MFD_CROS_EC_DEV=y
>>> CONFIG_COMMON_CLK_ZYNQMP=y
>>> CONFIG_SDM_GCC_845=y
>>> CONFIG_SM_GCC_8150=y
>>> CONFIG_SM_GCC_8250=y
>>> CONFIG_SLIMBUS=m
>>> CONFIG_INTERCONNECT=y
>>> CONFIG_CONFIGFS_FS=y
>>
>> All three above are still user-selectable, so please leave them. It is
>> redundant, but there is no guarantee that something selecting a
>> user-visible symbol will stop selecting it. IOW, user-visible symbols
>> should be still chosen by defconfigs if they really want them.
> 
> Well, but even if they are already enabled anyway? What is the point of savedefconfig then resp. you are saying
> that the commited defconfigs should not be generated using savedefconfig? That sounds rather confusing to me.

Yes, it's confusing. savedefconfig is too aggressive - it strips
everything which is not strictly necessary now. However there is no
guarantee that current select of some option, thus stripped out that
option via savedefconfig, will stay there forever.

If CONFIG_XXX is not user-selectable, then when someone removes the
"select XXX", it *is expected* to modify the defconfigs to add back this
XXX.

However it turns out that some developers assume that there is no such
requirement of re-adding XXX to defconfig, if XXX was user-selectable in
the first place.


> 
>> See for example commit a2315d3aea59 ("ARM: exynos_defconfig: Restore
>> debugfs support") for rationale why we need to keep them.
> 
> Okay, I see what you mean as in relation to above mentioned commit but then any change to Kconfig dependencies
> can ultimately change the behaviour of previous configs unless they are complete ones (or were just lucky to
> include whatever stuff that changed). But how should one now know which of them zillions of user-selectable
> options should be added to such "more robust" defconfigs?

No clue... :) Except maybe do not remove existing ones.

> 
> I suggest a better approach might be to have some CI which validates defconfig changes. That way one could
> easily track stuff disappearing and could at that point explicitly enable it again.

Yes, that makes sense. One defines the expected symbols to have and CI
validates whether they are enabled. We have something like this (called
annotations) in our Ubuntu configs - we store a list of config options
were are interested in and detect when they change.

Another approach is to use specialized fragments.

The benefits of such solutions is that they also document why such
option was chosen in the first place (what I was also asking from your
patches here at beginning and thanks for fixing this).

Best regards,
Krzysztof