| Message ID | 20230405-sunhme-includes-fix-v1-1-bf17cc5de20d@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | f8b648bf6628eb0a6c25be68e512bed6df2e5f24 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: sunhme: move asm includes to below linux includes | expand |
On 4/5/23 13:29, Simon Horman wrote: > A recent rearrangement of includes has lead to a problem on m68k > as flagged by the kernel test robot. > > Resolve this by moving the block asm includes to below linux includes. > A side effect i that non-Sparc asm includes are now immediately > before Sparc asm includes, which seems nice. > > Using sparse v0.6.4 I was able to reproduce this problem as follows > using the config provided by the kernel test robot: > > $ wget https://download.01.org/0day-ci/archive/20230404/202304041748.0sQc4K4l-lkp@intel.com/config > $ cp config .config > $ make ARCH=m68k oldconfig > $ make ARCH=m68k C=2 M=drivers/net/ethernet/sun > CC [M] drivers/net/ethernet/sun/sunhme.o > In file included from drivers/net/ethernet/sun/sunhme.c:19: > ./arch/m68k/include/asm/irq.h:78:11: error: expected ‘;’ before ‘void’ > 78 | asmlinkage void do_IRQ(int irq, struct pt_regs *regs); > | ^~~~~ > | ; > ./arch/m68k/include/asm/irq.h:78:40: warning: ‘struct pt_regs’ declared inside parameter list will not be visible outside of this definition or declaration > 78 | asmlinkage void do_IRQ(int irq, struct pt_regs *regs); > | ^~~~~~~ This seems like a problem with the header. m68k's asm/irq.h should include linux/interrupt.h before its declarations. --Sean > Compile tested only. > > Fixes: 1ff4f42aef60 ("net: sunhme: Alphabetize includes") > Reported-by: kernel test robot <lkp@intel.com> > Link: https://lore.kernel.org/oe-kbuild-all/202304041748.0sQc4K4l-lkp@intel.com/ > Signed-off-by: Simon Horman <horms@kernel.org> > --- > drivers/net/ethernet/sun/sunhme.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/ethernet/sun/sunhme.c b/drivers/net/ethernet/sun/sunhme.c > index ec85aef35bf9..b93613cd1994 100644 > --- a/drivers/net/ethernet/sun/sunhme.c > +++ b/drivers/net/ethernet/sun/sunhme.c > @@ -14,9 +14,6 @@ > * argument : macaddr=0x00,0x10,0x20,0x30,0x40,0x50 > */ > > -#include <asm/byteorder.h> > -#include <asm/dma.h> > -#include <asm/irq.h> > #include <linux/bitops.h> > #include <linux/crc32.h> > #include <linux/delay.h> > @@ -45,6 +42,10 @@ > #include <linux/types.h> > #include <linux/uaccess.h> > > +#include <asm/byteorder.h> > +#include <asm/dma.h> > +#include <asm/irq.h> > + > #ifdef CONFIG_SPARC > #include <asm/auxio.h> > #include <asm/idprom.h> >
On Wed, Apr 05, 2023 at 01:34:11PM -0400, Sean Anderson wrote: > On 4/5/23 13:29, Simon Horman wrote: > > A recent rearrangement of includes has lead to a problem on m68k > > as flagged by the kernel test robot. > > > > Resolve this by moving the block asm includes to below linux includes. > > A side effect i that non-Sparc asm includes are now immediately > > before Sparc asm includes, which seems nice. > > > > Using sparse v0.6.4 I was able to reproduce this problem as follows > > using the config provided by the kernel test robot: > > > > $ wget https://download.01.org/0day-ci/archive/20230404/202304041748.0sQc4K4l-lkp@intel.com/config > > $ cp config .config > > $ make ARCH=m68k oldconfig > > $ make ARCH=m68k C=2 M=drivers/net/ethernet/sun > > CC [M] drivers/net/ethernet/sun/sunhme.o > > In file included from drivers/net/ethernet/sun/sunhme.c:19: > > ./arch/m68k/include/asm/irq.h:78:11: error: expected ‘;’ before ‘void’ > > 78 | asmlinkage void do_IRQ(int irq, struct pt_regs *regs); > > | ^~~~~ > > | ; > > ./arch/m68k/include/asm/irq.h:78:40: warning: ‘struct pt_regs’ declared inside parameter list will not be visible outside of this definition or declaration > > 78 | asmlinkage void do_IRQ(int irq, struct pt_regs *regs); > > | ^~~~~~~ > > This seems like a problem with the header. m68k's asm/irq.h should include linux/interrupt.h before its declarations. Hi Sean, I do see your point. But TBH I'm unsure which way to go on this one. Geert, do you have any input? > --Sean > > > Compile tested only. > > > > Fixes: 1ff4f42aef60 ("net: sunhme: Alphabetize includes") > > Reported-by: kernel test robot <lkp@intel.com> > > Link: https://lore.kernel.org/oe-kbuild-all/202304041748.0sQc4K4l-lkp@intel.com/ > > Signed-off-by: Simon Horman <horms@kernel.org> > > --- > > drivers/net/ethernet/sun/sunhme.c | 7 ++++--- > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/net/ethernet/sun/sunhme.c b/drivers/net/ethernet/sun/sunhme.c > > index ec85aef35bf9..b93613cd1994 100644 > > --- a/drivers/net/ethernet/sun/sunhme.c > > +++ b/drivers/net/ethernet/sun/sunhme.c > > @@ -14,9 +14,6 @@ > > * argument : macaddr=0x00,0x10,0x20,0x30,0x40,0x50 > > */ > > -#include <asm/byteorder.h> > > -#include <asm/dma.h> > > -#include <asm/irq.h> > > #include <linux/bitops.h> > > #include <linux/crc32.h> > > #include <linux/delay.h> > > @@ -45,6 +42,10 @@ > > #include <linux/types.h> > > #include <linux/uaccess.h> > > +#include <asm/byteorder.h> > > +#include <asm/dma.h> > > +#include <asm/irq.h> > > + > > #ifdef CONFIG_SPARC > > #include <asm/auxio.h> > > #include <asm/idprom.h> > > >
On Wed, Apr 05, 2023 at 08:02:37PM +0200, Simon Horman wrote: > On Wed, Apr 05, 2023 at 01:34:11PM -0400, Sean Anderson wrote: > > On 4/5/23 13:29, Simon Horman wrote: > > > A recent rearrangement of includes has lead to a problem on m68k > > > as flagged by the kernel test robot. > > > > > > Resolve this by moving the block asm includes to below linux includes. > > > A side effect i that non-Sparc asm includes are now immediately > > > before Sparc asm includes, which seems nice. > > > > > > Using sparse v0.6.4 I was able to reproduce this problem as follows > > > using the config provided by the kernel test robot: > > > > > > $ wget https://download.01.org/0day-ci/archive/20230404/202304041748.0sQc4K4l-lkp@intel.com/config > > > $ cp config .config > > > $ make ARCH=m68k oldconfig > > > $ make ARCH=m68k C=2 M=drivers/net/ethernet/sun > > > CC [M] drivers/net/ethernet/sun/sunhme.o > > > In file included from drivers/net/ethernet/sun/sunhme.c:19: > > > ./arch/m68k/include/asm/irq.h:78:11: error: expected ‘;’ before ‘void’ > > > 78 | asmlinkage void do_IRQ(int irq, struct pt_regs *regs); > > > | ^~~~~ > > > | ; > > > ./arch/m68k/include/asm/irq.h:78:40: warning: ‘struct pt_regs’ declared inside parameter list will not be visible outside of this definition or declaration > > > 78 | asmlinkage void do_IRQ(int irq, struct pt_regs *regs); > > > | ^~~~~~~ > > > > This seems like a problem with the header. m68k's asm/irq.h should include linux/interrupt.h before its declarations. > > Hi Sean, > > I do see your point. But TBH I'm unsure which way to go on this one. > Geert, do you have any input? We always include linux/* headers before asm/*. The "sorting" of headers in this way was inappropriate.
On 4/5/23 14:07, Matthew Wilcox wrote: > On Wed, Apr 05, 2023 at 08:02:37PM +0200, Simon Horman wrote: >> On Wed, Apr 05, 2023 at 01:34:11PM -0400, Sean Anderson wrote: >>> On 4/5/23 13:29, Simon Horman wrote: >>>> A recent rearrangement of includes has lead to a problem on m68k >>>> as flagged by the kernel test robot. >>>> >>>> Resolve this by moving the block asm includes to below linux includes. >>>> A side effect i that non-Sparc asm includes are now immediately >>>> before Sparc asm includes, which seems nice. >>>> >>>> Using sparse v0.6.4 I was able to reproduce this problem as follows >>>> using the config provided by the kernel test robot: >>>> >>>> $ wget https://download.01.org/0day-ci/archive/20230404/202304041748.0sQc4K4l-lkp@intel.com/config >>>> $ cp config .config >>>> $ make ARCH=m68k oldconfig >>>> $ make ARCH=m68k C=2 M=drivers/net/ethernet/sun >>>> CC [M] drivers/net/ethernet/sun/sunhme.o >>>> In file included from drivers/net/ethernet/sun/sunhme.c:19: >>>> ./arch/m68k/include/asm/irq.h:78:11: error: expected ‘;’ before ‘void’ >>>> 78 | asmlinkage void do_IRQ(int irq, struct pt_regs *regs); >>>> | ^~~~~ >>>> | ; >>>> ./arch/m68k/include/asm/irq.h:78:40: warning: ‘struct pt_regs’ declared inside parameter list will not be visible outside of this definition or declaration >>>> 78 | asmlinkage void do_IRQ(int irq, struct pt_regs *regs); >>>> | ^~~~~~~ >>> >>> This seems like a problem with the header. m68k's asm/irq.h should include linux/interrupt.h before its declarations. >> >> Hi Sean, >> >> I do see your point. But TBH I'm unsure which way to go on this one. >> Geert, do you have any input? > > We always include linux/* headers before asm/*. The "sorting" of > headers in this way was inappropriate. Is this written down anywhere? I couldn't find it in Documentation/process... --Sean
On Wed, Apr 05, 2023 at 02:09:55PM -0400, Sean Anderson wrote: > On 4/5/23 14:07, Matthew Wilcox wrote: > > We always include linux/* headers before asm/*. The "sorting" of > > headers in this way was inappropriate. > > Is this written down anywhere? I couldn't find it in Documentation/process... Feel free to send a patch. Generally, it should be: #include <linux/foo.h> #include <linux/bar.h> #include <asm/baz.h> #include <asm/quux.h> #include "local.h" Some drivers do this a different way with a single local.h that includes all necessary includes. Also if <linux/foo.h> and <asm/foo.h> both exist, you should include <linux/foo.h> (which almost certainly includes <asm/foo.h>)
On Wed, 5 Apr 2023 19:34:38 +0100 Matthew Wilcox wrote: > On Wed, Apr 05, 2023 at 02:09:55PM -0400, Sean Anderson wrote: > > On 4/5/23 14:07, Matthew Wilcox wrote: > > > We always include linux/* headers before asm/*. The "sorting" of > > > headers in this way was inappropriate. > > > > Is this written down anywhere? I couldn't find it in Documentation/process... > > Feel free to send a patch. Patch to documentation, checkpatch or both :)
Hi Willy, On Wed, Apr 5, 2023 at 8:34 PM Matthew Wilcox <willy@infradead.org> wrote: > On Wed, Apr 05, 2023 at 02:09:55PM -0400, Sean Anderson wrote: > > On 4/5/23 14:07, Matthew Wilcox wrote: > > > We always include linux/* headers before asm/*. The "sorting" of > > > headers in this way was inappropriate. > > > > Is this written down anywhere? I couldn't find it in Documentation/process... > > Feel free to send a patch. Generally, it should be: > > #include <linux/foo.h> > #include <linux/bar.h> > > #include <asm/baz.h> > #include <asm/quux.h> > > #include "local.h" > > Some drivers do this a different way with a single local.h that includes > all necessary includes. > > Also if <linux/foo.h> and <asm/foo.h> both exist, you should include > <linux/foo.h> (which almost certainly includes <asm/foo.h>) Indeed. Usually <asm/foo.h> should not be included directly, except for a few exceptions like <asm/irq.h>. Witness e.g. the (violated a lot) comment at the top of include/linux/irq.h: /* * Please do not include this file in generic code. There is currently * no requirement for any architecture to implement anything held * within this file. * * Thanks. --rmk */ Gr{oetje,eeting}s, Geert
Hello: This patch was applied to netdev/net-next.git (main) by Jakub Kicinski <kuba@kernel.org>: On Wed, 05 Apr 2023 19:29:48 +0200 you wrote: > A recent rearrangement of includes has lead to a problem on m68k > as flagged by the kernel test robot. > > Resolve this by moving the block asm includes to below linux includes. > A side effect i that non-Sparc asm includes are now immediately > before Sparc asm includes, which seems nice. > > [...] Here is the summary with links: - [net-next] net: sunhme: move asm includes to below linux includes https://git.kernel.org/netdev/net-next/c/f8b648bf6628 You are awesome, thank you!
diff --git a/drivers/net/ethernet/sun/sunhme.c b/drivers/net/ethernet/sun/sunhme.c index ec85aef35bf9..b93613cd1994 100644 --- a/drivers/net/ethernet/sun/sunhme.c +++ b/drivers/net/ethernet/sun/sunhme.c @@ -14,9 +14,6 @@ * argument : macaddr=0x00,0x10,0x20,0x30,0x40,0x50 */ -#include <asm/byteorder.h> -#include <asm/dma.h> -#include <asm/irq.h> #include <linux/bitops.h> #include <linux/crc32.h> #include <linux/delay.h> @@ -45,6 +42,10 @@ #include <linux/types.h> #include <linux/uaccess.h> +#include <asm/byteorder.h> +#include <asm/dma.h> +#include <asm/irq.h> + #ifdef CONFIG_SPARC #include <asm/auxio.h> #include <asm/idprom.h>
A recent rearrangement of includes has lead to a problem on m68k as flagged by the kernel test robot. Resolve this by moving the block asm includes to below linux includes. A side effect i that non-Sparc asm includes are now immediately before Sparc asm includes, which seems nice. Using sparse v0.6.4 I was able to reproduce this problem as follows using the config provided by the kernel test robot: $ wget https://download.01.org/0day-ci/archive/20230404/202304041748.0sQc4K4l-lkp@intel.com/config $ cp config .config $ make ARCH=m68k oldconfig $ make ARCH=m68k C=2 M=drivers/net/ethernet/sun CC [M] drivers/net/ethernet/sun/sunhme.o In file included from drivers/net/ethernet/sun/sunhme.c:19: ./arch/m68k/include/asm/irq.h:78:11: error: expected ‘;’ before ‘void’ 78 | asmlinkage void do_IRQ(int irq, struct pt_regs *regs); | ^~~~~ | ; ./arch/m68k/include/asm/irq.h:78:40: warning: ‘struct pt_regs’ declared inside parameter list will not be visible outside of this definition or declaration 78 | asmlinkage void do_IRQ(int irq, struct pt_regs *regs); | ^~~~~~~ Compile tested only. Fixes: 1ff4f42aef60 ("net: sunhme: Alphabetize includes") Reported-by: kernel test robot <lkp@intel.com> Link: https://lore.kernel.org/oe-kbuild-all/202304041748.0sQc4K4l-lkp@intel.com/ Signed-off-by: Simon Horman <horms@kernel.org> --- drivers/net/ethernet/sun/sunhme.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)