[viro-vfs:work.alpha 5/8] arch/alpha/kernel/io.c:655:1: error: redefinition of 'scr_memcpyw'

[viro-vfs:work.alpha 5/8] arch/alpha/kernel/io.c:655:1: error: redefinition of 'scr_memcpyw'

[kernel test robot]

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git work.alpha
head:   267674e3b4fd1ff6cedf9b22cd304daa75297966
commit: 1fb71c4d2bcacd6510fbe411016475ccc15b1a03 [5/8] alpha: missing includes
config: alpha-allnoconfig (https://download.01.org/0day-ci/archive/20240128/202401280650.Us2Lrkgl-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240128/202401280650.Us2Lrkgl-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202401280650.Us2Lrkgl-lkp@intel.com/

All errors (new ones prefixed by >>):

>> arch/alpha/kernel/io.c:655:1: error: redefinition of 'scr_memcpyw'
     655 | scr_memcpyw(u16 *d, const u16 *s, unsigned int count)
         | ^~~~~~~~~~~
   In file included from arch/alpha/kernel/io.c:10:
   include/linux/vt_buffer.h:42:20: note: previous definition of 'scr_memcpyw' with type 'void(u16 *, const u16 *, unsigned int)' {aka 'void(short unsigned int *, const short unsigned int *, unsigned int)'}
      42 | static inline void scr_memcpyw(u16 *d, const u16 *s, unsigned int count)
         |                    ^~~~~~~~~~~


vim +/scr_memcpyw +655 arch/alpha/kernel/io.c

^1da177e4c3f41 Linus Torvalds 2005-04-16  650  
^1da177e4c3f41 Linus Torvalds 2005-04-16  651  /* A version of memcpy used by the vga console routines to move data around
^1da177e4c3f41 Linus Torvalds 2005-04-16  652     arbitrarily between screen and main memory.  */
^1da177e4c3f41 Linus Torvalds 2005-04-16  653  
^1da177e4c3f41 Linus Torvalds 2005-04-16  654  void
^1da177e4c3f41 Linus Torvalds 2005-04-16 @655  scr_memcpyw(u16 *d, const u16 *s, unsigned int count)
^1da177e4c3f41 Linus Torvalds 2005-04-16  656  {
^1da177e4c3f41 Linus Torvalds 2005-04-16  657  	const u16 __iomem *ios = (const u16 __iomem *) s;
^1da177e4c3f41 Linus Torvalds 2005-04-16  658  	u16 __iomem *iod = (u16 __iomem *) d;
^1da177e4c3f41 Linus Torvalds 2005-04-16  659  	int s_isio = __is_ioaddr(s);
^1da177e4c3f41 Linus Torvalds 2005-04-16  660  	int d_isio = __is_ioaddr(d);
^1da177e4c3f41 Linus Torvalds 2005-04-16  661  
^1da177e4c3f41 Linus Torvalds 2005-04-16  662  	if (s_isio) {
^1da177e4c3f41 Linus Torvalds 2005-04-16  663  		if (d_isio) {
^1da177e4c3f41 Linus Torvalds 2005-04-16  664  			/* FIXME: Should handle unaligned ops and
^1da177e4c3f41 Linus Torvalds 2005-04-16  665  			   operation widening.  */
^1da177e4c3f41 Linus Torvalds 2005-04-16  666  
^1da177e4c3f41 Linus Torvalds 2005-04-16  667  			count /= 2;
^1da177e4c3f41 Linus Torvalds 2005-04-16  668  			while (count--) {
^1da177e4c3f41 Linus Torvalds 2005-04-16  669  				u16 tmp = __raw_readw(ios++);
^1da177e4c3f41 Linus Torvalds 2005-04-16  670  				__raw_writew(tmp, iod++);
^1da177e4c3f41 Linus Torvalds 2005-04-16  671  			}
^1da177e4c3f41 Linus Torvalds 2005-04-16  672  		}
^1da177e4c3f41 Linus Torvalds 2005-04-16  673  		else
^1da177e4c3f41 Linus Torvalds 2005-04-16  674  			memcpy_fromio(d, ios, count);
^1da177e4c3f41 Linus Torvalds 2005-04-16  675  	} else {
^1da177e4c3f41 Linus Torvalds 2005-04-16  676  		if (d_isio)
^1da177e4c3f41 Linus Torvalds 2005-04-16  677  			memcpy_toio(iod, s, count);
^1da177e4c3f41 Linus Torvalds 2005-04-16  678  		else
^1da177e4c3f41 Linus Torvalds 2005-04-16  679  			memcpy(d, s, count);
^1da177e4c3f41 Linus Torvalds 2005-04-16  680  	}
^1da177e4c3f41 Linus Torvalds 2005-04-16  681  }
^1da177e4c3f41 Linus Torvalds 2005-04-16  682  

:::::: The code at line 655 was first introduced by commit
:::::: 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 Linux-2.6.12-rc2

:::::: TO: Linus Torvalds <torvalds@ppc970.osdl.org>
:::::: CC: Linus Torvalds <torvalds@ppc970.osdl.org>

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [viro-vfs:work.alpha 5/8] arch/alpha/kernel/io.c:655:1: error: redefinition of 'scr_memcpyw'

[Al Viro]

On Sun, Jan 28, 2024 at 06:59:08AM +0800, kernel test robot wrote:
> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git work.alpha
> head:   267674e3b4fd1ff6cedf9b22cd304daa75297966
> commit: 1fb71c4d2bcacd6510fbe411016475ccc15b1a03 [5/8] alpha: missing includes
> config: alpha-allnoconfig (https://download.01.org/0day-ci/archive/20240128/202401280650.Us2Lrkgl-lkp@intel.com/config)
> compiler: alpha-linux-gcc (GCC) 13.2.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240128/202401280650.Us2Lrkgl-lkp@intel.com/reproduce)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202401280650.Us2Lrkgl-lkp@intel.com/
> 
> All errors (new ones prefixed by >>):
> 
> >> arch/alpha/kernel/io.c:655:1: error: redefinition of 'scr_memcpyw'
>      655 | scr_memcpyw(u16 *d, const u16 *s, unsigned int count)
>          | ^~~~~~~~~~~
>    In file included from arch/alpha/kernel/io.c:10:
>    include/linux/vt_buffer.h:42:20: note: previous definition of 'scr_memcpyw' with type 'void(u16 *, const u16 *, unsigned int)' {aka 'void(short unsigned int *, const short unsigned int *, unsigned int)'}
>       42 | static inline void scr_memcpyw(u16 *d, const u16 *s, unsigned int count)
>          |                    ^~~~~~~~~~~

_Very_ interesting.  First of all, there are 3 users of scr_memcpyw() -
vt.c, vgacon.c and fbcon.c.  All of them are getting that thing via
vt_buffer.h.  There we have
* include of asm/vga.h, conditional upon VGA_CONSOLE | MDA_CONSOLE
* fallback definitions of scr_...() stuff, conditional upon the
corresponding VT_BUF_HAVE_... not being defined.

The thing is, VT_BUF_HAVE_... are defined in asm/vga.h, so if you don't
have VGA_CONSOLE or MDA_CONSOLE you are going to get the default ones.
In case of scr_memcpyw() it's going to end up with memcpy(); on alpha
that does *not* match the native scr_memcpyw() instance.

Since we have vga.h in mandatory-y, with asm-generic fallback being
reasonable enough...  Should that include of asm/vga.h be conditional
in the first place?

I'm really unfamiliar with that thing - the last time I'd looked at
anything related to virtual console had been back in '99 or so...

Re: [viro-vfs:work.alpha 5/8] arch/alpha/kernel/io.c:655:1: error: redefinition of 'scr_memcpyw'

[Linus Torvalds]

On Sun, 28 Jan 2024 at 13:15, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> The thing is, VT_BUF_HAVE_... are defined in asm/vga.h, so if you don't
> have VGA_CONSOLE or MDA_CONSOLE you are going to get the default ones.
> In case of scr_memcpyw() it's going to end up with memcpy(); on alpha
> that does *not* match the native scr_memcpyw() instance.
>
> Since we have vga.h in mandatory-y, with asm-generic fallback being
> reasonable enough...  Should that include of asm/vga.h be conditional
> in the first place?

It should be conditional, because that's the only case you want to
actually have that special scr_memsetw() etc.

I think the problem is that you added the vtbuf include to <asm/io.h>,
which gets included from VGA_H early, before vga.h has even had time
to tell people that it overrides those helper functions.

I assume that moving the

    #define VT_BUF_HAVE_RW
    #define VT_BUF_HAVE_MEMSETW
    #define VT_BUF_HAVE_MEMCPYW

to above the

    #include <asm/io.h>

fixes the build?

That said, a good alternative might be to just stop using 'inline' for
the default scr_memsetw() and scr_memcpyw() functions, make them real
functions, and mark them __weak.

Then architectures can override them much more easily, and inlining
them seems a bit pointless.

But I doubt it's even worth cleaning things up in this area.

             Linus

Re: [viro-vfs:work.alpha 5/8] arch/alpha/kernel/io.c:655:1: error: redefinition of 'scr_memcpyw'

[Al Viro]

On Sun, Jan 28, 2024 at 01:55:31PM -0800, Linus Torvalds wrote:
> On Sun, 28 Jan 2024 at 13:15, Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > The thing is, VT_BUF_HAVE_... are defined in asm/vga.h, so if you don't
> > have VGA_CONSOLE or MDA_CONSOLE you are going to get the default ones.
> > In case of scr_memcpyw() it's going to end up with memcpy(); on alpha
> > that does *not* match the native scr_memcpyw() instance.
> >
> > Since we have vga.h in mandatory-y, with asm-generic fallback being
> > reasonable enough...  Should that include of asm/vga.h be conditional
> > in the first place?
> 
> It should be conditional, because that's the only case you want to
> actually have that special scr_memsetw() etc.
> 
> I think the problem is that you added the vtbuf include to <asm/io.h>,
> which gets included from VGA_H early, before vga.h has even had time
> to tell people that it overrides those helper functions.

Nope.  It's arch/alpha/kernel/io.c growing an include of linux/vt_buffer.h
and blowing up on allnoconfig.  The reason for that include was the
"missing prototype" on scr_memcpyw() definition in there...

> I assume that moving the
> 
>     #define VT_BUF_HAVE_RW
>     #define VT_BUF_HAVE_MEMSETW
>     #define VT_BUF_HAVE_MEMCPYW
> 
> to above the
> 
>     #include <asm/io.h>
> 
> fixes the build?

No such thing...  I can add an explicit extern in io.c, but that's
really obnoxious ;-/

> That said, a good alternative might be to just stop using 'inline' for
> the default scr_memsetw() and scr_memcpyw() functions, make them real
> functions, and mark them __weak.
> 
> Then architectures can override them much more easily, and inlining
> them seems a bit pointless.
> 
> But I doubt it's even worth cleaning things up in this area.

Do we ever use that thing on iomem in non-VGA setups?

Re: [viro-vfs:work.alpha 5/8] arch/alpha/kernel/io.c:655:1: error: redefinition of 'scr_memcpyw'

[Linus Torvalds]

On Sun, 28 Jan 2024 at 14:09, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> Do we ever use that thing on iomem in non-VGA setups?

On iomem? No. But I think several of the routines just work on either
VGA screen memory directly (c->vc_origin = vga_vram_base), or normal
RAM (vc_allocate)

But who knows.. I *used* to know all this code, but happily no longer.

                Linus

Re: [viro-vfs:work.alpha 5/8] arch/alpha/kernel/io.c:655:1: error: redefinition of 'scr_memcpyw'

[Al Viro]

On Sun, Jan 28, 2024 at 02:39:35PM -0800, Linus Torvalds wrote:
> On Sun, 28 Jan 2024 at 14:09, Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > Do we ever use that thing on iomem in non-VGA setups?
> 
> On iomem? No. But I think several of the routines just work on either
> VGA screen memory directly (c->vc_origin = vga_vram_base), or normal
> RAM (vc_allocate)
> 
> But who knows.. I *used* to know all this code, but happily no longer.


I hadn't finished going through the callers, but AFAICS in !VGA && !MDA
case alpha should be just fine with the fallbacks, so we could just
put the instance in io.c under the same ifdef that pulls asm/vga.h into
linux/vt_buffer.h

Re: [viro-vfs:work.alpha 5/8] arch/alpha/kernel/io.c:655:1: error: redefinition of 'scr_memcpyw'

[Al Viro]

On Mon, Jan 29, 2024 at 02:13:09AM +0000, Al Viro wrote:
> On Sun, Jan 28, 2024 at 02:39:35PM -0800, Linus Torvalds wrote:
> > On Sun, 28 Jan 2024 at 14:09, Al Viro <viro@zeniv.linux.org.uk> wrote:
> > >
> > > Do we ever use that thing on iomem in non-VGA setups?
> > 
> > On iomem? No. But I think several of the routines just work on either
> > VGA screen memory directly (c->vc_origin = vga_vram_base), or normal
> > RAM (vc_allocate)
> > 
> > But who knows.. I *used* to know all this code, but happily no longer.
> 
> 
> I hadn't finished going through the callers, but AFAICS in !VGA && !MDA
> case alpha should be just fine with the fallbacks, so we could just
> put the instance in io.c under the same ifdef that pulls asm/vga.h into
> linux/vt_buffer.h

BTW, fun observation:

#ifndef VT_BUF_HAVE_MEMCPYW
static inline void scr_memcpyw(u16 *d, const u16 *s, unsigned int count)
{
#ifdef VT_BUF_HAVE_RW
	count /= 2;
	while (count--)
		scr_writew(scr_readw(s++), d++);
#else
	memcpy(d, s, count);
#endif
}
#endif

is... interesting.  VT_BUF_HAVE_MEMCPYW is defined in asm/vga.h on alpha,
mips, powerpc and sparc.  VT_BUF_HAVE_RW is defined precisely in the
same cases.  In other words, fallback is *always* memcpy().  Now, note
that
arch/mips/include/asm/vga.h:50:#define scr_memcpyw(d, s, c) memcpy(d, s, c)
arch/powerpc/include/asm/vga.h:45:#define scr_memcpyw   memcpy
and on sparc we have
static inline void scr_memcpyw(u16 *d, u16 *s, unsigned int n)
{
        BUG_ON((long) d >= 0);

        memcpy(d, s, n);
}

IOW, we might as well kill VT_BUF_HAVE_MEMCPYW on everything except
alpha and turn the bit in vt_buffer.h into
#ifndef VT_BUF_HAVE_MEMCPYW
#define scr_memcpyw memcpyw
#endif

Furthermore, scr_memmovew() situation is not far from that - we
have
* architectures other than alpha/mips/powerpc/sparc - all end
up with memmove(), since neither VT_BUF_HAVE_MEMMOVEW nor
VT_BUF_HAVE_RW is defined
* mips/powerpc/sparc - memmove(), since it's explicitly defined
that way.
* alpha - that weird shit.

Incidentally, why do we play with asm/vga.h on sparc?  Had there ever
been sparc boxen with VGA card in them that would be in text mode?