| Message ID | 1658681004-132191-1-git-send-email-john.garry@huawei.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | arm64 defconfig: Get faddr2line working | expand |
On Sun, Jul 24, 2022 at 6:43 PM John Garry <john.garry@huawei.com> wrote: > > scripts/faddr2line has not worked by default for arm64 since 5.17 > > Firstly, since commit f9b3cd245784 ("Kconfig.debug: make DEBUG_INFO > selectable from a choice"), CONFIG_DEBUG_INFO was not getting enabled by > default (and this is required for faddr2line to work). I just noticed this the other day and applied a patch for it in the arm/defconfig branch. > Secondly, commit dcea997beed6 ("faddr2line: Fix overlapping text section > failures, the sequel") caused a breakage for arm64, as reported at the > following: > https://lore.kernel.org/lkml/3bd9817d-1959-c081-e5d0-8b0e70b3f41e@huawei.com/ > > Josh has sent fixes/improvements for faddr2line at the following: > https://lore.kernel.org/lkml/cover.1658426357.git.jpoimboe@kernel.org/ > > In this series I enable CONFIG_DEBUG_INFO by enabling > CONFIG_DEBUG_INFO_DWARF_TOOLCHAIN_DEFAULT, which seems the sensible > option. > > As for merging this, I am not sure who would pick it up - any takers > welcome. > > Note: this is based on next-20220722 and it may be wiser to sync the > defconfig manually (instead of using 1/2). Indeed I am not sure what is > the policy is of sync'ing this anyway. I only synchronized the 32-bit defconfig files in my tree, not the 64-bit one. However, I can't really apply your patch 2/2 because you appear to mix refreshing the order of the options with changes that remove options that are gone after a 'savedefconfig', risking that we miss other bugs as well, as seen from your diffstat: 1 file changed, 36 insertions(+), 48 deletions(-) I have refreshed this one as well now, which on my tree gives me 1 file changed, 31 insertions(+), 31 deletions(-) for a nonfunction change. I have left the other ones untouched for the moment: CONFIG_ARCH_BCMBCA=y CONFIG_SECCOMP=y CONFIG_QRTR=m CONFIG_PINCTRL_MSM=y CONFIG_SND_SOC_TEGRA210_OPE=m CONFIG_MAILBOX=y CONFIG_QCOM_ICC_BWMON=m CONFIG_SLIMBUS=m CONFIG_INTERCONNECT=y CONFIG_CONFIGFS_FS=y These should be checked manually to find out why savedefconfig no longer shows them, it could be either a bug (a new dependency, renamed option, a driver randomly selects another subsystem, etc) that we need to fix, or a harmless change (driver was removed, option is now intended to be default-enabled, ...) If you want to help more, can you check some or all of the above and send patches to either re-enable the options or remove them individually with explanations about why they are no longer part of the savedefconfig output? Arnd
On 24/07/2022 21:35, Arnd Bergmann wrote: >> Note: this is based on next-20220722 and it may be wiser to sync the >> defconfig manually (instead of using 1/2). Indeed I am not sure what is >> the policy is of sync'ing this anyway. > I only synchronized the 32-bit defconfig files in my tree, not the 64-bit > one. However, I can't really apply your patch 2/2 because you appear > to mix refreshing the order of the options with changes that remove > options that are gone after a 'savedefconfig', risking that we miss > other bugs as well, as seen from your diffstat: > > 1 file changed, 36 insertions(+), 48 deletions(-) > > I have refreshed this one as well now, which on my tree gives me > > 1 file changed, 31 insertions(+), 31 deletions(-) I am not sure what you are doing in this refresh - can you share the steps? I guess that you sync with the savedefconfig and then manually edit the resultant defconfig to restore the configs which were getting deleting (and not just moved around). For me - as you may expect - I do the following for the sync: make defconfig make savedefconfig mv defconfig arch/arm64/configs/defconfig > > for a nonfunction change. I have left the other ones untouched > for the moment: > > CONFIG_ARCH_BCMBCA=y > CONFIG_SECCOMP=y > CONFIG_QRTR=m > CONFIG_PINCTRL_MSM=y > CONFIG_SND_SOC_TEGRA210_OPE=m > CONFIG_MAILBOX=y > CONFIG_QCOM_ICC_BWMON=m > CONFIG_SLIMBUS=m > CONFIG_INTERCONNECT=y > CONFIG_CONFIGFS_FS=y > > These should be checked manually to find out why savedefconfig > no longer shows them, it could be either a bug (a new dependency, > renamed option, a driver randomly selects another subsystem, etc) > that we need to fix, or a harmless change (driver was removed, > option is now intended to be default-enabled, ...) > > If you want to help more, can you check some or all of the above > and send patches to either re-enable the options or remove them > individually with explanations about why they are no longer > part of the savedefconfig output? ok, I can check them. Thanks, John
On Mon, Jul 25, 2022 at 8:50 AM John Garry <john.garry@huawei.com> wrote: > On 24/07/2022 21:35, Arnd Bergmann wrote: > >> Note: this is based on next-20220722 and it may be wiser to sync the > >> defconfig manually (instead of using 1/2). Indeed I am not sure what is > >> the policy is of sync'ing this anyway. > > I only synchronized the 32-bit defconfig files in my tree, not the 64-bit > > one. However, I can't really apply your patch 2/2 because you appear > > to mix refreshing the order of the options with changes that remove > > options that are gone after a 'savedefconfig', risking that we miss > > other bugs as well, as seen from your diffstat: > > > > 1 file changed, 36 insertions(+), 48 deletions(-) > > > > I have refreshed this one as well now, which on my tree gives me > > > > 1 file changed, 31 insertions(+), 31 deletions(-) > > I am not sure what you are doing in this refresh - can you share the > steps? I guess that you sync with the savedefconfig and then manually > edit the resultant defconfig to restore the configs which were getting > deleting (and not just moved around). Yes, I did it manually using vimdiff here. I had scripted this for the arm32 defconfigs, but my script was a bit fragile so this seemed easier. You can find my commit at https://git.kernel.org/pub/scm/linux/kernel/git/soc/soc.git/commit/?h=arm/defconfig&id=abf73c76121d8417998356a1cccfccd17f5cfd11 > > These should be checked manually to find out why savedefconfig > > no longer shows them, it could be either a bug (a new dependency, > > renamed option, a driver randomly selects another subsystem, etc) > > that we need to fix, or a harmless change (driver was removed, > > option is now intended to be default-enabled, ...) > > > > If you want to help more, can you check some or all of the above > > and send patches to either re-enable the options or remove them > > individually with explanations about why they are no longer > > part of the savedefconfig output? > > ok, I can check them. Thanks! Arnd
On 24/07/2022 21:35, Arnd Bergmann wrote: Here's a brief'ish summary for why it's ok to delete these mentioned configs: > CONFIG_ARCH_BCMBCA=y On next-20220722, config ARCH_BCMBCA is selected by config ARCH_BCM4908 from arch/arm64/Kconfig.platforms and the latter is enabled in the defconfig, so no need to explicitly enable in the defconfig. On arm-soc arm/defconfig, config ARCH_BCMBCA does not exist for arm64 platforms yet, so this is why we see this config deleted for the sync. > CONFIG_SECCOMP=y Since commit 282a181b1a0d, config SECCOMP was changed to def_bool y, so no need to explicitly enable in the defconfig. > CONFIG_QRTR=m From commit fdf4632aa834, we enable config ATH11K_PCI, which selects config QRTR, so no need to explicitly enable in the defconfig. > 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. > 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. > 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 :) > 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 > 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. > 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 > 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. 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. So let me know if any comments or how to proceed. And would each config item deletion merit a separate patch? Thanks, John
On Mon, Jul 25, 2022 at 1:57 PM John Garry <john.garry@huawei.com> wrote: > > On 24/07/2022 21:35, Arnd Bergmann wrote: > > Here's a brief'ish summary for why it's ok to delete these mentioned > configs: > > > CONFIG_ARCH_BCMBCA=y > > On next-20220722, config ARCH_BCMBCA is selected by config ARCH_BCM4908 > from arch/arm64/Kconfig.platforms and the latter is enabled in the > defconfig, so no need to explicitly enable in the defconfig. > > On arm-soc arm/defconfig, config ARCH_BCMBCA does not exist for arm64 > platforms yet, so this is why we see this config deleted for the sync. > > > CONFIG_SECCOMP=y > > Since commit 282a181b1a0d, config SECCOMP was changed to def_bool y, so > no need to explicitly enable in the defconfig. Ok > > CONFIG_QRTR=m > > From commit fdf4632aa834, we enable config ATH11K_PCI, which selects > 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. > > 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'. > > 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. > > 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 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. > > 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. 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. > 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. Arnd
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
On Mon, Jul 25, 2022 at 4:03 PM John Garry <john.garry@huawei.com> wrote: > > > >>> 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_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 a couple of trade-offs between the two approaches. The main advantage of 'select' is that you can enable drivers more easily and all the required subsystems are there automatically. The advantage of 'depends on' is that it becomes easier to disable entire subsystems that one may not need. Which of those two is more important is of course a matter of perspective, I like to be able to turn things off more easily because that makes it possible to test the corner cases with randconfig more easily, and it helps produce size-reduced kernels for embedded systems. Another aspect is that we overall have more 'depends on' than 'select', and sticking with the more common way avoids circular dependencies, both within an area of the kernel and overall. The rule that I tend to follow with 'select' is to only use it on symbols that you don't even want to show to users. If a feature is part of a library (think zlib), then each user just needs to select the symbol but you never actually have to decide whether to show it or not. > >> 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). The CONFIG_DEBUG_INFO one should be fixed by my series from last week already, do you still see another issue with that? I actually have another patch to fix up all the non-Arm defconfigs for this one as well, but haven't sent that one yet. Arnd
On 25/07/2022 15:22, Arnd Bergmann wrote: >> INTERCONNECT has no dependencies, so "select" - like for MAILBOX - >> should be fine, I suppose > There are a couple of trade-offs between the two approaches. > The main advantage of 'select' is that you can enable drivers more > easily and all the required subsystems are there automatically. > The advantage of 'depends on' is that it becomes easier to disable > entire subsystems that one may not need. > > Which of those two is more important is of course a matter of perspective, > I like to be able to turn things off more easily because that makes it > possible to test the corner cases with randconfig more easily, and it > helps produce size-reduced kernels for embedded systems. > > Another aspect is that we overall have more 'depends on' than 'select', > and sticking with the more common way avoids circular dependencies, > both within an area of the kernel and overall. > > The rule that I tend to follow with 'select' is to only use it on symbols > that you don't even want to show to users. If a feature is part of > a library (think zlib), then each user just needs to select the symbol > but you never actually have to decide whether to show it or not. ok, seems reasonable. Personally I dislike 'select' for all the common reasons. > >>>> 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). > The CONFIG_DEBUG_INFO one should be fixed by my series from > last week already, do you still see another issue with that? Ah, I thought that you re-enabled CONFIG_DEBUG_INFO for only the arm32 configs but see that you also re-enabled for arm64 defconfig as well. > I actually > have another patch to fix up all the non-Arm defconfigs for this one as > well, but haven't sent that one yet. Thanks, John
trim list On 25/07/2022 13:51, Arnd Bergmann wrote: >>> 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 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. > JFYI, building for CONFIG_SOUNDWIRE_QCOM=y and CONFIG_SLIBMUS=m is ok. The driver has a runtime check for CONFIG_SLIMBUS in qcom_swrm_probe(). That implementation seems consistent with guidance in kconfig-language.rst - so do you think that there is still a problem? thanks, John
On Thu, Jul 28, 2022 at 10:06 AM John Garry <john.garry@huawei.com> wrote: > > trim list Adding a few other people for slimbus. > On 25/07/2022 13:51, Arnd Bergmann wrote: > >>> 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 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. > > > > JFYI, building for CONFIG_SOUNDWIRE_QCOM=y and CONFIG_SLIBMUS=m is ok. > The driver has a runtime check for CONFIG_SLIMBUS in qcom_swrm_probe(). > > That implementation seems consistent with guidance in > kconfig-language.rst - so do you think that there is still a problem? I see Vinod added the IS_REACHABLE() check, which makes it build, but I think both the 'imply' and the IS_REACHABLE() are mistakes here, as they are almost everywhere else. From what I understand, the driver can actually use two different back-ends, with slimbus being one of them. In the configuration with CONFIG_SOUNDWIRE_QCOM=y and CONFIG_SLIBMUS=m, I think you can end up with a slimbus device being detected at runtime, which gets passed to the built-in qcom driver, but that driver then misdetects the bus because it is not linked against the slimbus module. It then uses the incorrect code path, causing unintended behavior. The 'depends on SLIMBUS || !SLIMBUS' dependency would completely avoid this issue and make the driver work correctly in each configuration. Arnd
On 28/07/2022 09:23, Arnd Bergmann wrote: > On Thu, Jul 28, 2022 at 10:06 AM John Garry <john.garry@huawei.com> wrote: >> >> trim list > > Adding a few other people for slimbus. > >> On 25/07/2022 13:51, Arnd Bergmann wrote: >>>>> 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 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. >>> >> >> JFYI, building for CONFIG_SOUNDWIRE_QCOM=y and CONFIG_SLIBMUS=m is ok. >> The driver has a runtime check for CONFIG_SLIMBUS in qcom_swrm_probe(). >> >> That implementation seems consistent with guidance in >> kconfig-language.rst - so do you think that there is still a problem? > > I see Vinod added the IS_REACHABLE() check, which makes it build, but > I think both the 'imply' and the IS_REACHABLE() are mistakes here, as they > are almost everywhere else. > > From what I understand, the driver can actually use two different > back-ends, with slimbus being one of them. In the configuration > with CONFIG_SOUNDWIRE_QCOM=y and CONFIG_SLIBMUS=m, w.r.t Qualcomm SoundWire Controller. There are two types of integrations at IP level. 1> SoundWire Controller is part of Codec which is connected over SlimBus. ex: SoCs SDM845 and older ones All the read writes have to go via SlimBus physical layer. 2> SoundWire Controller is part of Codec which is MMIO,ex: SM8250 and newer SoCs. No SlimBus dependency. As we are using a common driver for this, we wanted to check if the parent was a slimbus or not, to be able to select correct read/write interfaces. rethinking about this approach, It should be doable to also derive this information from compatible strings, Soundwire controller versions 1.3.0 and below are always of type 1 and the newer ones are type 2. So this could get rid of IS_REACHABLE check along with references to slimbus_bus from driver. expressing the SlimBus dependency for type 1 should be either captured using imply SLIMBUS or depends in Kconfig. > I think you can end up with a slimbus device being detected at > runtime, which gets passed to the built-in qcom driver, but that driver > then misdetects the bus because it is not linked against the > slimbus module. It then uses the incorrect code path, causing > unintended behavior. > > The 'depends on SLIMBUS || !SLIMBUS' dependency would completely > avoid this issue and make the driver work correctly in each configuration. > > Arnd Thanks, Srini
On Thu, Jul 28, 2022 at 10:53 AM Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote: > On 28/07/2022 09:23, Arnd Bergmann wrote: > > On Thu, Jul 28, 2022 at 10:06 AM John Garry <john.garry@huawei.com> wrote: > > As we are using a common driver for this, we wanted to check if the > parent was a slimbus or not, to be able to select correct read/write > interfaces. > > rethinking about this approach, It should be doable to also derive this > information from compatible strings, Soundwire controller versions 1.3.0 > and below are always of type 1 and the newer ones are type 2. > > So this could get rid of IS_REACHABLE check along with references to > slimbus_bus from driver. Referencing slimbus_bus here is not a problem, just use IS_ENABLED() and fix the dependency. While at it, please also > expressing the SlimBus dependency for type 1 should be either captured > using imply SLIMBUS or depends in Kconfig. 'imply' has no effect on compile testing, it only affects what goes in the defconfig file, so there has a be a 'depends on'. To allow building both with and without slimbus, but disallow the configuration with slimbus as a loadable module and soundwire-qcom as built-in, do it like this: diff --git a/drivers/soundwire/Kconfig b/drivers/soundwire/Kconfig index 2b7795233282..69e46cce7d8b 100644 --- a/drivers/soundwire/Kconfig +++ b/drivers/soundwire/Kconfig @@ -35,7 +35,7 @@ config SOUNDWIRE_INTEL config SOUNDWIRE_QCOM tristate "Qualcomm SoundWire Master driver" - imply SLIMBUS + depends on SLIMBUS || !SLIMBUS depends on SND_SOC help SoundWire Qualcomm Master driver. diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c index 22b706350ead..f65c7737c2db 100644 --- a/drivers/soundwire/qcom.c +++ b/drivers/soundwire/qcom.c @@ -1289,11 +1289,7 @@ static int qcom_swrm_probe(struct platform_device *pdev) data = of_device_get_match_data(dev); ctrl->rows_index = sdw_find_row_index(data->default_rows); ctrl->cols_index = sdw_find_col_index(data->default_cols); -#if IS_REACHABLE(CONFIG_SLIMBUS) - if (dev->parent->bus == &slimbus_bus) { -#else - if (false) { -#endif + if (IS_ENABLED(CONFIG_SLIMBUS) && dev->parent->bus == &slimbus_bus) { ctrl->reg_read = qcom_swrm_ahb_reg_read; ctrl->reg_write = qcom_swrm_ahb_reg_write; ctrl->regmap = dev_get_regmap(dev->parent, NULL);
On 7/28/22 5:09 AM, Arnd Bergmann wrote: ... > diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c > index 22b706350ead..f65c7737c2db 100644 > --- a/drivers/soundwire/qcom.c > +++ b/drivers/soundwire/qcom.c > @@ -1289,11 +1289,7 @@ static int qcom_swrm_probe(struct platform_device *pdev) > data = of_device_get_match_data(dev); > ctrl->rows_index = sdw_find_row_index(data->default_rows); > ctrl->cols_index = sdw_find_col_index(data->default_cols); > -#if IS_REACHABLE(CONFIG_SLIMBUS) > - if (dev->parent->bus == &slimbus_bus) { > -#else > - if (false) { > -#endif > + if (IS_ENABLED(CONFIG_SLIMBUS) && dev->parent->bus == &slimbus_bus) { > ctrl->reg_read = qcom_swrm_ahb_reg_read; > ctrl->reg_write = qcom_swrm_ahb_reg_write; > ctrl->regmap = dev_get_regmap(dev->parent, NULL); > This relies on the compiler optimizing out the reference to slimbus_bus, which doesn't exist in the CONFIG_SLIMBUS=n case (not tested, but I think this means an O0 build will fail?).
On Thu, Jul 28, 2022 at 1:57 PM Jonathan Marek <jonathan@marek.ca> wrote: > On 7/28/22 5:09 AM, Arnd Bergmann wrote: > > diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c > > index 22b706350ead..f65c7737c2db 100644 > > --- a/drivers/soundwire/qcom.c > > +++ b/drivers/soundwire/qcom.c > > @@ -1289,11 +1289,7 @@ static int qcom_swrm_probe(struct platform_device *pdev) > > data = of_device_get_match_data(dev); > > ctrl->rows_index = sdw_find_row_index(data->default_rows); > > ctrl->cols_index = sdw_find_col_index(data->default_cols); > > -#if IS_REACHABLE(CONFIG_SLIMBUS) > > - if (dev->parent->bus == &slimbus_bus) { > > -#else > > - if (false) { > > -#endif > > + if (IS_ENABLED(CONFIG_SLIMBUS) && dev->parent->bus == &slimbus_bus) { > > ctrl->reg_read = qcom_swrm_ahb_reg_read; > > ctrl->reg_write = qcom_swrm_ahb_reg_write; > > ctrl->regmap = dev_get_regmap(dev->parent, NULL); > > > > This relies on the compiler optimizing out the reference to slimbus_bus, > which doesn't exist in the CONFIG_SLIMBUS=n case (not tested, but I > think this means an O0 build will fail?). That is correct: We rely on this everywhere in the kernel, which is why it is impossible to build kernels with -O0. Arnd
On 28/07/2022 13:03, Arnd Bergmann wrote: > On Thu, Jul 28, 2022 at 1:57 PM Jonathan Marek <jonathan@marek.ca> wrote: >> On 7/28/22 5:09 AM, Arnd Bergmann wrote: >>> diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c >>> index 22b706350ead..f65c7737c2db 100644 >>> --- a/drivers/soundwire/qcom.c >>> +++ b/drivers/soundwire/qcom.c >>> @@ -1289,11 +1289,7 @@ static int qcom_swrm_probe(struct platform_device *pdev) >>> data = of_device_get_match_data(dev); >>> ctrl->rows_index = sdw_find_row_index(data->default_rows); >>> ctrl->cols_index = sdw_find_col_index(data->default_cols); >>> -#if IS_REACHABLE(CONFIG_SLIMBUS) >>> - if (dev->parent->bus == &slimbus_bus) { >>> -#else >>> - if (false) { >>> -#endif >>> + if (IS_ENABLED(CONFIG_SLIMBUS) && dev->parent->bus == &slimbus_bus) { >>> ctrl->reg_read = qcom_swrm_ahb_reg_read; >>> ctrl->reg_write = qcom_swrm_ahb_reg_write; >>> ctrl->regmap = dev_get_regmap(dev->parent, NULL); >>> >> >> This relies on the compiler optimizing out the reference to slimbus_bus, >> which doesn't exist in the CONFIG_SLIMBUS=n case (not tested, but I >> think this means an O0 build will fail?). > > That is correct: We rely on this everywhere in the kernel, which is why > it is impossible to build kernels with -O0. > Was there any patch sent to fix/improve this? I could not see any. Or any plan to do so? Thanks, John
On 04/08/2022 11:35, John Garry wrote: >>>> ctrl->cols_index = sdw_find_col_index(data->default_cols); >>>> -#if IS_REACHABLE(CONFIG_SLIMBUS) >>>> - if (dev->parent->bus == &slimbus_bus) { >>>> -#else >>>> - if (false) { >>>> -#endif >>>> + if (IS_ENABLED(CONFIG_SLIMBUS) && dev->parent->bus == >>>> &slimbus_bus) { >>>> ctrl->reg_read = qcom_swrm_ahb_reg_read; >>>> ctrl->reg_write = qcom_swrm_ahb_reg_write; >>>> ctrl->regmap = dev_get_regmap(dev->parent, NULL); >>>> >>> >>> This relies on the compiler optimizing out the reference to slimbus_bus, >>> which doesn't exist in the CONFIG_SLIMBUS=n case (not tested, but I >>> think this means an O0 build will fail?). >> >> That is correct: We rely on this everywhere in the kernel, which is why >> it is impossible to build kernels with -O0. >> > > Was there any patch sent to fix/improve this? I could not see any. Or > any plan to do so? Since I got no response, I'll assume 'no'. So I'll look to do this when I get a chance. Thanks, John
scripts/faddr2line has not worked by default for arm64 since 5.17 Firstly, since commit f9b3cd245784 ("Kconfig.debug: make DEBUG_INFO selectable from a choice"), CONFIG_DEBUG_INFO was not getting enabled by default (and this is required for faddr2line to work). Secondly, commit dcea997beed6 ("faddr2line: Fix overlapping text section failures, the sequel") caused a breakage for arm64, as reported at the following: https://lore.kernel.org/lkml/3bd9817d-1959-c081-e5d0-8b0e70b3f41e@huawei.com/ Josh has sent fixes/improvements for faddr2line at the following: https://lore.kernel.org/lkml/cover.1658426357.git.jpoimboe@kernel.org/ In this series I enable CONFIG_DEBUG_INFO by enabling CONFIG_DEBUG_INFO_DWARF_TOOLCHAIN_DEFAULT, which seems the sensible option. As for merging this, I am not sure who would pick it up - any takers welcome. Note: this is based on next-20220722 and it may be wiser to sync the defconfig manually (instead of using 1/2). Indeed I am not sure what is the policy is of sync'ing this anyway. Thanks! John Garry (2): arm64: defconfig: Sync with savedefconfig arm64: defconfig: Enable DEBUG_INFO_DWARF_TOOLCHAIN_DEFAULT arch/arm64/configs/defconfig | 85 ++++++++++++++++-------------------- 1 file changed, 37 insertions(+), 48 deletions(-)