| Message ID | 20240502123449.2690-1-puranjay@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/2] arm64/arch_timer: include <linux/percpu.h> | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On 5/2/24 18:04, Puranjay Mohan wrote: > arch_timer.h includes linux/smp.h to use DEFINE_PER_CPU() and it works > because smp.h includes percpu.h. The next commit will remove percpu.h > from smp.h and it will break this usage. > > Explicitly include percpu.h and remove smp.h But this particular change does not seem to be necessary for changing raw_smp_processor_id() as current_thread_info()->cpu being done in the later patch ? You might still leave header <asm/percpu.h> inclusion in arch/arm64/include/asm/smp.h while dropping the per cpu cpu_number ? > > Signed-off-by: Puranjay Mohan <puranjay@kernel.org> > --- > arch/arm64/include/asm/arch_timer.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h > index 934c658ee947..f5794d50f51d 100644 > --- a/arch/arm64/include/asm/arch_timer.h > +++ b/arch/arm64/include/asm/arch_timer.h > @@ -15,7 +15,7 @@ > #include <linux/bug.h> > #include <linux/init.h> > #include <linux/jump_label.h> > -#include <linux/smp.h> > +#include <linux/percpu.h> > #include <linux/types.h> > > #include <clocksource/arm_arch_timer.h>
Anshuman Khandual <anshuman.khandual@arm.com> writes: > On 5/2/24 18:04, Puranjay Mohan wrote: >> arch_timer.h includes linux/smp.h to use DEFINE_PER_CPU() and it works >> because smp.h includes percpu.h. The next commit will remove percpu.h >> from smp.h and it will break this usage. >> >> Explicitly include percpu.h and remove smp.h > > But this particular change does not seem to be necessary for changing > raw_smp_processor_id() as current_thread_info()->cpu being done in the > later patch ? You might still leave header <asm/percpu.h> inclusion in > arch/arm64/include/asm/smp.h while dropping the per cpu cpu_number ? commit 57c82954e77f ("arm64: make cpu number a percpu variable") created this percpu variable and included <asm/percpu.h> in <asm/smp.h> Now we are removing the percpu variable cpu_number from smp.h, so there is no need to keep percpu.h in smp.h I feel users of DECLARE_PER_CPU_[...], etc. should include percpu.h and not smp.h as it makes reading the code more easier and can thwart build issues in the future, when headers are changed. Thanks, Puranjay
On Thu, May 02, 2024 at 12:34:48PM +0000, Puranjay Mohan wrote: > arch_timer.h includes linux/smp.h to use DEFINE_PER_CPU() and it works > because smp.h includes percpu.h. The next commit will remove percpu.h > from smp.h and it will break this usage. > > Explicitly include percpu.h and remove smp.h > > Signed-off-by: Puranjay Mohan <puranjay@kernel.org> Looks like this was a thinko in commit: 6acc71ccac7187fc ("arm64: arch_timer: Allows a CPU-specific erratum to only affect a subset of CPUs") ... which, as you say, should have included <linux/percpu.h> rather than <linux/smp.h>. This is a cleanup regardless of the next patch. Acked-by: Mark Rutland <mark.rutland@arm.com> Mark. > --- > arch/arm64/include/asm/arch_timer.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h > index 934c658ee947..f5794d50f51d 100644 > --- a/arch/arm64/include/asm/arch_timer.h > +++ b/arch/arm64/include/asm/arch_timer.h > @@ -15,7 +15,7 @@ > #include <linux/bug.h> > #include <linux/init.h> > #include <linux/jump_label.h> > -#include <linux/smp.h> > +#include <linux/percpu.h> > #include <linux/types.h> > > #include <clocksource/arm_arch_timer.h> > -- > 2.40.1 >
On Fri, May 03, 2024 at 02:37:45PM +0530, Anshuman Khandual wrote: > > > On 5/2/24 18:04, Puranjay Mohan wrote: > > arch_timer.h includes linux/smp.h to use DEFINE_PER_CPU() and it works > > because smp.h includes percpu.h. The next commit will remove percpu.h > > from smp.h and it will break this usage. > > > > Explicitly include percpu.h and remove smp.h > > But this particular change does not seem to be necessary for changing > raw_smp_processor_id() as current_thread_info()->cpu being done in the > later patch ? You might still leave header <asm/percpu.h> inclusion in > arch/arm64/include/asm/smp.h while dropping the per cpu cpu_number ? Why would that be preferable? The general rule is that if a file uses something explicitly, it should include the relevant header directly rather than something that happens to transitively include that header. We made a mistake and included the wrong header in commit: 6acc71ccac7187fc ("arm64: arch_timer: Allows a CPU-specific erratum to only affect a subset of CPUs") ... so we should fix that regardless of the next patch. The point of the next patch is to effectively revert commit: 57c82954e77fa12c ("arm64: make cpu number a percpu variable") ... and reverting that means we should stop including <asm/percpu.h> from <asm/smp.h>; anything depending on that is already doing something wrong, and leaving the include there only serves to paper over bugs. Mark. > > > > > Signed-off-by: Puranjay Mohan <puranjay@kernel.org> > > --- > > arch/arm64/include/asm/arch_timer.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h > > index 934c658ee947..f5794d50f51d 100644 > > --- a/arch/arm64/include/asm/arch_timer.h > > +++ b/arch/arm64/include/asm/arch_timer.h > > @@ -15,7 +15,7 @@ > > #include <linux/bug.h> > > #include <linux/init.h> > > #include <linux/jump_label.h> > > -#include <linux/smp.h> > > +#include <linux/percpu.h> > > #include <linux/types.h> > > > > #include <clocksource/arm_arch_timer.h>
On 5/3/24 15:14, Puranjay Mohan wrote: > Anshuman Khandual <anshuman.khandual@arm.com> writes: > >> On 5/2/24 18:04, Puranjay Mohan wrote: >>> arch_timer.h includes linux/smp.h to use DEFINE_PER_CPU() and it works >>> because smp.h includes percpu.h. The next commit will remove percpu.h >>> from smp.h and it will break this usage. >>> >>> Explicitly include percpu.h and remove smp.h >> >> But this particular change does not seem to be necessary for changing >> raw_smp_processor_id() as current_thread_info()->cpu being done in the >> later patch ? You might still leave header <asm/percpu.h> inclusion in >> arch/arm64/include/asm/smp.h while dropping the per cpu cpu_number ? > > commit 57c82954e77f ("arm64: make cpu number a percpu variable") > created this percpu variable and included <asm/percpu.h> in <asm/smp.h> > > Now we are removing the percpu variable cpu_number from smp.h, so there > is no need to keep percpu.h in smp.h Fair enough. > > I feel users of DECLARE_PER_CPU_[...], etc. should include percpu.h and > not smp.h as it makes reading the code more easier and can thwart build > issues in the future, when headers are changed. Right, makes sense, hope there is no more such cases using smp.h to pull in DECLARE_PER_CPU_[...]. A quick build on defconfig is successful after this patch.
On 5/2/24 18:04, Puranjay Mohan wrote: > arch_timer.h includes linux/smp.h to use DEFINE_PER_CPU() and it works > because smp.h includes percpu.h. The next commit will remove percpu.h > from smp.h and it will break this usage. > > Explicitly include percpu.h and remove smp.h > > Signed-off-by: Puranjay Mohan <puranjay@kernel.org> Reviewed-by: Anshuman Khandual <anshuman.khandual@arm.com> > --- > arch/arm64/include/asm/arch_timer.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h > index 934c658ee947..f5794d50f51d 100644 > --- a/arch/arm64/include/asm/arch_timer.h > +++ b/arch/arm64/include/asm/arch_timer.h > @@ -15,7 +15,7 @@ > #include <linux/bug.h> > #include <linux/init.h> > #include <linux/jump_label.h> > -#include <linux/smp.h> > +#include <linux/percpu.h> > #include <linux/types.h> > > #include <clocksource/arm_arch_timer.h>
diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h index 934c658ee947..f5794d50f51d 100644 --- a/arch/arm64/include/asm/arch_timer.h +++ b/arch/arm64/include/asm/arch_timer.h @@ -15,7 +15,7 @@ #include <linux/bug.h> #include <linux/init.h> #include <linux/jump_label.h> -#include <linux/smp.h> +#include <linux/percpu.h> #include <linux/types.h> #include <clocksource/arm_arch_timer.h>
arch_timer.h includes linux/smp.h to use DEFINE_PER_CPU() and it works because smp.h includes percpu.h. The next commit will remove percpu.h from smp.h and it will break this usage. Explicitly include percpu.h and remove smp.h Signed-off-by: Puranjay Mohan <puranjay@kernel.org> --- arch/arm64/include/asm/arch_timer.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)