| Message ID | 20230126163530.3495413-1-arnd@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Coresight: tpda/tpdm: remove incorrect __exit annotation | expand |
Hi Arnd, On 26/01/2023 16:35, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > 'remove' callbacks get called whenever a device is unbound from > the driver, which can get triggered from user space. > > Putting it into the __exit section means that the function gets > dropped in for built-in drivers, as pointed out by this build > warning: > > `tpda_remove' referenced in section `.data' of drivers/hwtracing/coresight/coresight-tpda.o: defined in discarded section `.exit.text' of drivers/hwtracing/coresight/coresight-tpda.o > `tpdm_remove' referenced in section `.data' of drivers/hwtracing/coresight/coresight-tpdm.o: defined in discarded section `.exit.text' of drivers/hwtracing/coresight/coresight-tpdm.o > Thanks for the fix, I will queue this. Btw, I did try to reproduce it locally, but couldn't trigger the warnings, even with CONFIG_WERROR=y and all CORESIGHT configs builtin. I see other drivers doing the same outside coresight too. Just curious to know why is this any different. Is it specific to "bus" driver (e.g. AMBA) ? Suzuki > Fixes: 5b7916625c01 ("Coresight: Add TPDA link driver") > Fixes: b3c71626a933 ("Coresight: Add coresight TPDM source driver") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/hwtracing/coresight/coresight-tpda.c | 2 +- > drivers/hwtracing/coresight/coresight-tpdm.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight-tpda.c b/drivers/hwtracing/coresight/coresight-tpda.c > index 19c25c9f6157..382d648529e7 100644 > --- a/drivers/hwtracing/coresight/coresight-tpda.c > +++ b/drivers/hwtracing/coresight/coresight-tpda.c > @@ -174,7 +174,7 @@ static int tpda_probe(struct amba_device *adev, const struct amba_id *id) > return 0; > } > > -static void __exit tpda_remove(struct amba_device *adev) > +static void tpda_remove(struct amba_device *adev) > { > struct tpda_drvdata *drvdata = dev_get_drvdata(&adev->dev); > > diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c > index 349a82bb3270..9479a5e8c672 100644 > --- a/drivers/hwtracing/coresight/coresight-tpdm.c > +++ b/drivers/hwtracing/coresight/coresight-tpdm.c > @@ -223,7 +223,7 @@ static int tpdm_probe(struct amba_device *adev, const struct amba_id *id) > return 0; > } > > -static void __exit tpdm_remove(struct amba_device *adev) > +static void tpdm_remove(struct amba_device *adev) > { > struct tpdm_drvdata *drvdata = dev_get_drvdata(&adev->dev); >
On Thu, Jan 26, 2023, at 19:02, Suzuki K Poulose wrote: > On 26/01/2023 16:35, Arnd Bergmann wrote: >> From: Arnd Bergmann <arnd@arndb.de> > Thanks for the fix, I will queue this. Btw, I did try to > reproduce it locally, but couldn't trigger the warnings, > even with > > CONFIG_WERROR=y > > and all CORESIGHT configs builtin. I see other drivers doing the > same outside coresight too. Just curious to know why is this > any different. Is it specific to "bus" driver (e.g. AMBA) ? The warning comes from postprocessing the object file, it's got nothing to do with the bus type, only with a symbol in .data referencing a symbol in .init.text. Maybe there are some config options that keep the section from getting discarded? Or possibly you only built the files in this directory, but did not get to the final link? Arnd
On 26/01/2023 20:37, Arnd Bergmann wrote: > On Thu, Jan 26, 2023, at 19:02, Suzuki K Poulose wrote: >> On 26/01/2023 16:35, Arnd Bergmann wrote: >>> From: Arnd Bergmann <arnd@arndb.de> >> Thanks for the fix, I will queue this. Btw, I did try to >> reproduce it locally, but couldn't trigger the warnings, >> even with >> >> CONFIG_WERROR=y >> >> and all CORESIGHT configs builtin. I see other drivers doing the >> same outside coresight too. Just curious to know why is this >> any different. Is it specific to "bus" driver (e.g. AMBA) ? > > The warning comes from postprocessing the object file, it's got > nothing to do with the bus type, only with a symbol in .data > referencing a symbol in .init.text. Maybe there are some > config options that keep the section from getting discarded? > Or possibly you only built the files in this directory, but did > not get to the final link? I did a full kernel build. Also, I see a similar issue with the coresight-etm4x (by code inspection) driver. Did you not hit that ? May be there is a config option that is masking it on my end. But the case of etm4x driver is puzzling. $ git grep etm4_remove_amba drivers/hwtracing/coresight/coresight-etm4x-core.c drivers/hwtracing/coresight/coresight-etm4x-core.c:static void __exit etm4_remove_amba(struct amba_device *adev) drivers/hwtracing/coresight/coresight-etm4x-core.c: .remove = etm4_remove_amba, Suzuki > > Arnd
On Fri, Jan 27, 2023, at 17:46, Suzuki K Poulose wrote: > On 26/01/2023 20:37, Arnd Bergmann wrote: >> On Thu, Jan 26, 2023, at 19:02, Suzuki K Poulose wrote: >>> On 26/01/2023 16:35, Arnd Bergmann wrote: >>>> From: Arnd Bergmann <arnd@arndb.de> >>> Thanks for the fix, I will queue this. Btw, I did try to >>> reproduce it locally, but couldn't trigger the warnings, >>> even with >>> >>> CONFIG_WERROR=y >>> >>> and all CORESIGHT configs builtin. I see other drivers doing the >>> same outside coresight too. Just curious to know why is this >>> any different. Is it specific to "bus" driver (e.g. AMBA) ? >> >> The warning comes from postprocessing the object file, it's got >> nothing to do with the bus type, only with a symbol in .data >> referencing a symbol in .init.text. Maybe there are some >> config options that keep the section from getting discarded? >> Or possibly you only built the files in this directory, but did >> not get to the final link? > > I did a full kernel build. Also, I see a similar issue with the > coresight-etm4x (by code inspection) driver. Did you not hit that ? > > May be there is a config option that is masking it on my end. But > the case of etm4x driver is puzzling. > > $ git grep etm4_remove_amba > drivers/hwtracing/coresight/coresight-etm4x-core.c > drivers/hwtracing/coresight/coresight-etm4x-core.c:static void __exit > etm4_remove_amba(struct amba_device *adev) > drivers/hwtracing/coresight/coresight-etm4x-core.c: .remove > = etm4_remove_amba, Indeed, that one clearly has the same but, but I have never observed a warning for it. I checked one more thing and found that I only get the warning for 32-bit Arm builds, but not arm64. Since the etm4x driver 'depends on ARM64' for its use of asm/sysreg.h, I never test-built it on 32-bit arm. From the git history of arch/arm64/kernel/vmlinux.lds.S, I can see that arm64 never discards the .exit section, see commit 07c802bd7c39 ("arm64: vmlinux.lds.S: don't discard .exit.* sections at link-time"). Arnd
On 27/01/2023 17:00, Arnd Bergmann wrote: > On Fri, Jan 27, 2023, at 17:46, Suzuki K Poulose wrote: >> On 26/01/2023 20:37, Arnd Bergmann wrote: >>> On Thu, Jan 26, 2023, at 19:02, Suzuki K Poulose wrote: >>>> On 26/01/2023 16:35, Arnd Bergmann wrote: >>>>> From: Arnd Bergmann <arnd@arndb.de> >>>> Thanks for the fix, I will queue this. Btw, I did try to >>>> reproduce it locally, but couldn't trigger the warnings, >>>> even with >>>> >>>> CONFIG_WERROR=y >>>> >>>> and all CORESIGHT configs builtin. I see other drivers doing the >>>> same outside coresight too. Just curious to know why is this >>>> any different. Is it specific to "bus" driver (e.g. AMBA) ? >>> >>> The warning comes from postprocessing the object file, it's got >>> nothing to do with the bus type, only with a symbol in .data >>> referencing a symbol in .init.text. Maybe there are some >>> config options that keep the section from getting discarded? >>> Or possibly you only built the files in this directory, but did >>> not get to the final link? >> >> I did a full kernel build. Also, I see a similar issue with the >> coresight-etm4x (by code inspection) driver. Did you not hit that ? >> >> May be there is a config option that is masking it on my end. But >> the case of etm4x driver is puzzling. >> >> $ git grep etm4_remove_amba >> drivers/hwtracing/coresight/coresight-etm4x-core.c >> drivers/hwtracing/coresight/coresight-etm4x-core.c:static void __exit >> etm4_remove_amba(struct amba_device *adev) >> drivers/hwtracing/coresight/coresight-etm4x-core.c: .remove >> = etm4_remove_amba, > > Indeed, that one clearly has the same but, but I have never > observed a warning for it. > > I checked one more thing and found that I only get the warning > for 32-bit Arm builds, but not arm64. Since the etm4x driver > 'depends on ARM64' for its use of asm/sysreg.h, > I never test-built it on 32-bit arm. > > From the git history of arch/arm64/kernel/vmlinux.lds.S, > I can see that arm64 never discards the .exit section, see > commit 07c802bd7c39 ("arm64: vmlinux.lds.S: don't discard > .exit.* sections at link-time"). That makes sense, thanks for getting to the bottom of this. I have pushed it to coresight next. https://git.kernel.org/coresight/c/0c1ccc158bbc Kind regards Suzuki > > Arnd
diff --git a/drivers/hwtracing/coresight/coresight-tpda.c b/drivers/hwtracing/coresight/coresight-tpda.c index 19c25c9f6157..382d648529e7 100644 --- a/drivers/hwtracing/coresight/coresight-tpda.c +++ b/drivers/hwtracing/coresight/coresight-tpda.c @@ -174,7 +174,7 @@ static int tpda_probe(struct amba_device *adev, const struct amba_id *id) return 0; } -static void __exit tpda_remove(struct amba_device *adev) +static void tpda_remove(struct amba_device *adev) { struct tpda_drvdata *drvdata = dev_get_drvdata(&adev->dev); diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c index 349a82bb3270..9479a5e8c672 100644 --- a/drivers/hwtracing/coresight/coresight-tpdm.c +++ b/drivers/hwtracing/coresight/coresight-tpdm.c @@ -223,7 +223,7 @@ static int tpdm_probe(struct amba_device *adev, const struct amba_id *id) return 0; } -static void __exit tpdm_remove(struct amba_device *adev) +static void tpdm_remove(struct amba_device *adev) { struct tpdm_drvdata *drvdata = dev_get_drvdata(&adev->dev);