| Message ID | 20231212155407.1429121-9-james.clark@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | coresight: Separate sysfs and Perf usage and some other cleanups | expand |
Hi James On 12/12/2023 15:54, James Clark wrote: > These are a bit annoying to keep up to date when the function signatures > change. But if CONFIG_CORESIGHT isn't enabled, then they're not used > anyway so just delete them. > Have you tried building an arm32 kernel with this change in ? Looks like arch/arm/kernel/hw_breakpoint.c includes linux/coresight.h and a build with CONFIG_CORSIGHT=n might break the build ? So is drivers/accel/habanalabs/common/habanalabs.h. Now, I am not sure if they really need it (even if they do, we may be able to remove the dependency on the header file. Suzuki > Signed-off-by: James Clark <james.clark@arm.com> > --- > include/linux/coresight.h | 79 --------------------------------------- > 1 file changed, 79 deletions(-) > > diff --git a/include/linux/coresight.h b/include/linux/coresight.h > index 4400d554a16b..c5be46d7f85c 100644 > --- a/include/linux/coresight.h > +++ b/include/linux/coresight.h > @@ -391,8 +391,6 @@ struct coresight_ops { > const struct coresight_ops_helper *helper_ops; > }; > > -#if IS_ENABLED(CONFIG_CORESIGHT) > - > static inline u32 csdev_access_relaxed_read32(struct csdev_access *csa, > u32 offset) > { > @@ -611,83 +609,6 @@ void coresight_relaxed_write64(struct coresight_device *csdev, > u64 val, u32 offset); > void coresight_write64(struct coresight_device *csdev, u64 val, u32 offset); > > -#else > -static inline struct coresight_device * > -coresight_register(struct coresight_desc *desc) { return NULL; } > -static inline void coresight_unregister(struct coresight_device *csdev) {} > -static inline int > -coresight_enable_sysfs(struct coresight_device *csdev) { return -ENOSYS; } > -static inline void coresight_disable_sysfs(struct coresight_device *csdev) {} > - > -static inline int coresight_timeout(struct csdev_access *csa, u32 offset, > - int position, int value) > -{ > - return 1; > -} > - > -static inline int coresight_claim_device_unlocked(struct coresight_device *csdev) > -{ > - return -EINVAL; > -} > - > -static inline int coresight_claim_device(struct coresight_device *csdev) > -{ > - return -EINVAL; > -} > - > -static inline void coresight_disclaim_device(struct coresight_device *csdev) {} > -static inline void coresight_disclaim_device_unlocked(struct coresight_device *csdev) {} > - > -static inline bool coresight_loses_context_with_cpu(struct device *dev) > -{ > - return false; > -} > - > -static inline u32 coresight_relaxed_read32(struct coresight_device *csdev, u32 offset) > -{ > - WARN_ON_ONCE(1); > - return 0; > -} > - > -static inline u32 coresight_read32(struct coresight_device *csdev, u32 offset) > -{ > - WARN_ON_ONCE(1); > - return 0; > -} > - > -static inline void coresight_write32(struct coresight_device *csdev, u32 val, u32 offset) > -{ > -} > - > -static inline void coresight_relaxed_write32(struct coresight_device *csdev, > - u32 val, u32 offset) > -{ > -} > - > -static inline u64 coresight_relaxed_read64(struct coresight_device *csdev, > - u32 offset) > -{ > - WARN_ON_ONCE(1); > - return 0; > -} > - > -static inline u64 coresight_read64(struct coresight_device *csdev, u32 offset) > -{ > - WARN_ON_ONCE(1); > - return 0; > -} > - > -static inline void coresight_relaxed_write64(struct coresight_device *csdev, > - u64 val, u32 offset) > -{ > -} > - > -static inline void coresight_write64(struct coresight_device *csdev, u64 val, u32 offset) > -{ > -} > - > -#endif /* IS_ENABLED(CONFIG_CORESIGHT) */ > - > extern int coresight_get_cpu(struct device *dev); > > struct coresight_platform_data *coresight_get_platform_data(struct device *dev);
On 09/01/2024 10:38, Suzuki K Poulose wrote: > Hi James > > On 12/12/2023 15:54, James Clark wrote: >> These are a bit annoying to keep up to date when the function signatures >> change. But if CONFIG_CORESIGHT isn't enabled, then they're not used >> anyway so just delete them. >> > > Have you tried building an arm32 kernel with this change in ? Looks like > arch/arm/kernel/hw_breakpoint.c includes linux/coresight.h and a build > with CONFIG_CORSIGHT=n might break the build ? So is arm32 and CONFIG_CORESIGHT=n works because hw_breakpoint.c doesn't use any of those symbols, only #defines that were outside the #if IS_ENABLED(CONFIG_CORESIGHT), specifically CORESIGHT_UNLOCK. > drivers/accel/habanalabs/common/habanalabs.h. Now, I am not sure if they habanalabs is interesting, it depends on X86_64, but CONFIG_CORESIGHT depends on ARM || ARM64, so I think we can assume it's also only looking for #defines and inlines, and not actual code. Either way I can't find any build config that actually ever built this, meaning it's always been dead code. I would have expected some build robot to have flagged an error by now as I've seen that on other coresight patches. > really need it (even if they do, we may be able to remove the dependency > on the header file. > They do really need it, also for the CORESIGHT_UNLOCK definition, but not any functions. > Suzuki > >> Signed-off-by: James Clark <james.clark@arm.com> >> --- >> include/linux/coresight.h | 79 --------------------------------------- >> 1 file changed, 79 deletions(-) >> >> diff --git a/include/linux/coresight.h b/include/linux/coresight.h >> index 4400d554a16b..c5be46d7f85c 100644 >> --- a/include/linux/coresight.h >> +++ b/include/linux/coresight.h >> @@ -391,8 +391,6 @@ struct coresight_ops { >> const struct coresight_ops_helper *helper_ops; >> }; >> -#if IS_ENABLED(CONFIG_CORESIGHT) >> - >> static inline u32 csdev_access_relaxed_read32(struct csdev_access *csa, >> u32 offset) >> { >> @@ -611,83 +609,6 @@ void coresight_relaxed_write64(struct >> coresight_device *csdev, >> u64 val, u32 offset); >> void coresight_write64(struct coresight_device *csdev, u64 val, u32 >> offset); >> -#else >> -static inline struct coresight_device * >> -coresight_register(struct coresight_desc *desc) { return NULL; } >> -static inline void coresight_unregister(struct coresight_device >> *csdev) {} >> -static inline int >> -coresight_enable_sysfs(struct coresight_device *csdev) { return >> -ENOSYS; } >> -static inline void coresight_disable_sysfs(struct coresight_device >> *csdev) {} >> - >> -static inline int coresight_timeout(struct csdev_access *csa, u32 >> offset, >> - int position, int value) >> -{ >> - return 1; >> -} >> - >> -static inline int coresight_claim_device_unlocked(struct >> coresight_device *csdev) >> -{ >> - return -EINVAL; >> -} >> - >> -static inline int coresight_claim_device(struct coresight_device *csdev) >> -{ >> - return -EINVAL; >> -} >> - >> -static inline void coresight_disclaim_device(struct coresight_device >> *csdev) {} >> -static inline void coresight_disclaim_device_unlocked(struct >> coresight_device *csdev) {} >> - >> -static inline bool coresight_loses_context_with_cpu(struct device *dev) >> -{ >> - return false; >> -} >> - >> -static inline u32 coresight_relaxed_read32(struct coresight_device >> *csdev, u32 offset) >> -{ >> - WARN_ON_ONCE(1); >> - return 0; >> -} >> - >> -static inline u32 coresight_read32(struct coresight_device *csdev, >> u32 offset) >> -{ >> - WARN_ON_ONCE(1); >> - return 0; >> -} >> - >> -static inline void coresight_write32(struct coresight_device *csdev, >> u32 val, u32 offset) >> -{ >> -} >> - >> -static inline void coresight_relaxed_write32(struct coresight_device >> *csdev, >> - u32 val, u32 offset) >> -{ >> -} >> - >> -static inline u64 coresight_relaxed_read64(struct coresight_device >> *csdev, >> - u32 offset) >> -{ >> - WARN_ON_ONCE(1); >> - return 0; >> -} >> - >> -static inline u64 coresight_read64(struct coresight_device *csdev, >> u32 offset) >> -{ >> - WARN_ON_ONCE(1); >> - return 0; >> -} >> - >> -static inline void coresight_relaxed_write64(struct coresight_device >> *csdev, >> - u64 val, u32 offset) >> -{ >> -} >> - >> -static inline void coresight_write64(struct coresight_device *csdev, >> u64 val, u32 offset) >> -{ >> -} >> - >> -#endif /* IS_ENABLED(CONFIG_CORESIGHT) */ >> - >> extern int coresight_get_cpu(struct device *dev); >> struct coresight_platform_data *coresight_get_platform_data(struct >> device *dev); >
On 09/01/2024 16:48, James Clark wrote: > > > On 09/01/2024 10:38, Suzuki K Poulose wrote: >> Hi James >> >> On 12/12/2023 15:54, James Clark wrote: >>> These are a bit annoying to keep up to date when the function signatures >>> change. But if CONFIG_CORESIGHT isn't enabled, then they're not used >>> anyway so just delete them. >>> >> >> Have you tried building an arm32 kernel with this change in ? Looks like >> arch/arm/kernel/hw_breakpoint.c includes linux/coresight.h and a build >> with CONFIG_CORSIGHT=n might break the build ? So is > > arm32 and CONFIG_CORESIGHT=n works because hw_breakpoint.c doesn't use > any of those symbols, only #defines that were outside the #if > IS_ENABLED(CONFIG_CORESIGHT), specifically CORESIGHT_UNLOCK. > >> drivers/accel/habanalabs/common/habanalabs.h. Now, I am not sure if they > > habanalabs is interesting, it depends on X86_64, but CONFIG_CORESIGHT > depends on ARM || ARM64, so I think we can assume it's also only looking > for #defines and inlines, and not actual code. > > Either way I can't find any build config that actually ever built this, > meaning it's always been dead code. I would have expected some build > robot to have flagged an error by now as I've seen that on other > coresight patches. > >> really need it (even if they do, we may be able to remove the dependency >> on the header file. >> > > They do really need it, also for the CORESIGHT_UNLOCK definition, but > not any functions. Thanks for checking this. Suzuki
diff --git a/include/linux/coresight.h b/include/linux/coresight.h index 4400d554a16b..c5be46d7f85c 100644 --- a/include/linux/coresight.h +++ b/include/linux/coresight.h @@ -391,8 +391,6 @@ struct coresight_ops { const struct coresight_ops_helper *helper_ops; }; -#if IS_ENABLED(CONFIG_CORESIGHT) - static inline u32 csdev_access_relaxed_read32(struct csdev_access *csa, u32 offset) { @@ -611,83 +609,6 @@ void coresight_relaxed_write64(struct coresight_device *csdev, u64 val, u32 offset); void coresight_write64(struct coresight_device *csdev, u64 val, u32 offset); -#else -static inline struct coresight_device * -coresight_register(struct coresight_desc *desc) { return NULL; } -static inline void coresight_unregister(struct coresight_device *csdev) {} -static inline int -coresight_enable_sysfs(struct coresight_device *csdev) { return -ENOSYS; } -static inline void coresight_disable_sysfs(struct coresight_device *csdev) {} - -static inline int coresight_timeout(struct csdev_access *csa, u32 offset, - int position, int value) -{ - return 1; -} - -static inline int coresight_claim_device_unlocked(struct coresight_device *csdev) -{ - return -EINVAL; -} - -static inline int coresight_claim_device(struct coresight_device *csdev) -{ - return -EINVAL; -} - -static inline void coresight_disclaim_device(struct coresight_device *csdev) {} -static inline void coresight_disclaim_device_unlocked(struct coresight_device *csdev) {} - -static inline bool coresight_loses_context_with_cpu(struct device *dev) -{ - return false; -} - -static inline u32 coresight_relaxed_read32(struct coresight_device *csdev, u32 offset) -{ - WARN_ON_ONCE(1); - return 0; -} - -static inline u32 coresight_read32(struct coresight_device *csdev, u32 offset) -{ - WARN_ON_ONCE(1); - return 0; -} - -static inline void coresight_write32(struct coresight_device *csdev, u32 val, u32 offset) -{ -} - -static inline void coresight_relaxed_write32(struct coresight_device *csdev, - u32 val, u32 offset) -{ -} - -static inline u64 coresight_relaxed_read64(struct coresight_device *csdev, - u32 offset) -{ - WARN_ON_ONCE(1); - return 0; -} - -static inline u64 coresight_read64(struct coresight_device *csdev, u32 offset) -{ - WARN_ON_ONCE(1); - return 0; -} - -static inline void coresight_relaxed_write64(struct coresight_device *csdev, - u64 val, u32 offset) -{ -} - -static inline void coresight_write64(struct coresight_device *csdev, u64 val, u32 offset) -{ -} - -#endif /* IS_ENABLED(CONFIG_CORESIGHT) */ - extern int coresight_get_cpu(struct device *dev); struct coresight_platform_data *coresight_get_platform_data(struct device *dev);
These are a bit annoying to keep up to date when the function signatures change. But if CONFIG_CORESIGHT isn't enabled, then they're not used anyway so just delete them. Signed-off-by: James Clark <james.clark@arm.com> --- include/linux/coresight.h | 79 --------------------------------------- 1 file changed, 79 deletions(-)