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/[email protected]/
Josh has sent fixes/improvements for faddr2line at the following:
https://lore.kernel.org/lkml/[email protected]/
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(-)
--
2.35.3
Since commit f9b3cd245784 ("Kconfig.debug: make DEBUG_INFO selectable
from a choice") faddr2line has stopped working by default as it relies
on CONFIG_DEBUG_INFO.
Enable CONFIG_DEBUG_INFO_DWARF_TOOLCHAIN_DEFAULT to get it working again,
which seems the sensible config option to use.
Signed-off-by: John Garry <[email protected]>
---
arch/arm64/configs/defconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index de662de0bb4d..43e3550afa45 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -1311,6 +1311,7 @@ CONFIG_CRYPTO_DEV_HISI_TRNG=m
CONFIG_CMA_SIZE_MBYTES=32
CONFIG_PRINTK_TIME=y
CONFIG_DEBUG_KERNEL=y
+CONFIG_DEBUG_INFO_DWARF_TOOLCHAIN_DEFAULT=y
CONFIG_MAGIC_SYSRQ=y
CONFIG_DEBUG_FS=y
# CONFIG_SCHED_DEBUG is not set
--
2.35.3
On Sun, Jul 24, 2022 at 6:43 PM John Garry <[email protected]> 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/[email protected]/
>
> Josh has sent fixes/improvements for faddr2line at the following:
> https://lore.kernel.org/lkml/[email protected]/
>
> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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
<[email protected]> wrote:
> On 28/07/2022 09:23, Arnd Bergmann wrote:
> > On Thu, Jul 28, 2022 at 10:06 AM John Garry <[email protected]> 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 Thu, Jul 28, 2022 at 1:57 PM Jonathan Marek <[email protected]> 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 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 28/07/2022 13:03, Arnd Bergmann wrote:
> On Thu, Jul 28, 2022 at 1:57 PM Jonathan Marek <[email protected]> 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