checkpatch nits ...

checkpatch nits ...

[Kevin Diggs]

Hi,

Can I ignore these checkpatch errors:

ERROR: do not initialise statics to 0 or NULL
#829: FILE: powerpc/kernel/cpu/pll_if.c:61:
+static unsigned int override_bus_clock = 0;

ERROR: do not initialise externals to 0 or NULL
#1281: FILE: powerpc/kernel/cpu/pll_if.c:513:
+int rval = 0;

Someone (Arnd?) told me this was due to an older compiler putting these
in a strange section?

WARNING: externs should be avoided in .c files
#1137: FILE: powerpc/kernel/cpu/pll_if.c:369:
+       __asm__ __volatile__ (

??? I don't know what this is?

The entire block is:

	__asm__ __volatile__ (
		"addi %0,%3,-1\n"
		"andc %1,%3,%0\n"
		"cntlzw %1,%1\n"
		"subfic %1,%1,31\n"
		"cntlzw %0,%2\n":
		"=r"(cntlz), "=r"(cnttz):
		"r"(tmp), "b"(cnttz)
	);

Re: checkpatch nits ...

[Sean MacLennan]

On Fri, 22 Aug 2008 17:21:44 -0700
"Kevin Diggs" <kevdig@hypersurf.com> wrote:

> Hi,
> 
> Can I ignore these checkpatch errors:
> 
> ERROR: do not initialise statics to 0 or NULL
> #829: FILE: powerpc/kernel/cpu/pll_if.c:61:
> +static unsigned int override_bus_clock = 0;
> 
> ERROR: do not initialise externals to 0 or NULL
> #1281: FILE: powerpc/kernel/cpu/pll_if.c:513:
> +int rval = 0;
> 
> Someone (Arnd?) told me this was due to an older compiler putting
> these in a strange section?

Older gcc, and in fact many (most?) compilers, put *all* initialized
variables in a data section rather than the bss. This means they took
up room in the executable. By not explicitly setting them to zero, they
where put in the bss and initialized to zero anyway.

Newer gccs will put them in the bss if they are zero. You could argue
that this is technically wrong.

I tend to remove the 0s or nulls just to cut down on the checkpatch
errors. You need to decide if it is worth it. I wouldn't submit a patch
with *just* these changes.

Cheers,
   Sean

Re: checkpatch nits ...

[Arnd Bergmann]

On Saturday 23 August 2008, Kevin Diggs wrote:
> WARNING: externs should be avoided in .c files
> #1137: FILE: powerpc/kernel/cpu/pll_if.c:369:
> + =A0 =A0 =A0 __asm__ __volatile__ (
>=20
> ??? I don't know what this is?
>=20
> The entire block is:
>=20
> =A0=A0=A0=A0=A0=A0=A0=A0__asm__ __volatile__ (
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0"addi %0,%3,-1\n"
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0"andc %1,%3,%0\n"
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0"cntlzw %1,%1\n"
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0"subfic %1,%1,31\n"
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0"cntlzw %0,%2\n":
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0"=3Dr"(cntlz), "=3Dr"(cnt=
tz):
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0"r"(tmp), "b"(cnttz)
> =A0=A0=A0=A0=A0=A0=A0=A0);
>=20

It's a bug in checkpatch, your code is correct (although I would write
asm volatile, not __asm__ __volatile__, and add \t after each \n).
Can you explain why you need that inline assembly? All you do in there
are arithmetic operations, so you should be able to express that using
C, or at least using the helpers we already have.

Checkpatch thinks that what you wrote is a declaration for a function
named __volatile__ returning a variable of type __asm__, and complains
that this declaration belongs into a header file.

	Arnd <><

Re: checkpatch nits ...

[David Howells]

Kevin Diggs <kevdig@hypersurf.com> wrote:

> The entire block is:
> 
> 	__asm__ __volatile__ (
> 		"addi %0,%3,-1\n"
> 		"andc %1,%3,%0\n"
> 		"cntlzw %1,%1\n"
> 		"subfic %1,%1,31\n"
> 		"cntlzw %0,%2\n":
> 		"=r"(cntlz), "=r"(cnttz):
> 		"r"(tmp), "b"(cnttz)
> 	);

As long as this has no side effects, the __volatile__ isn't necessary.  If the
only effect is to produce the specified outputs based on the specified inputs,
the code can be moved or eliminated if the outputs aren't used.

David

Re: checkpatch nits ...

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1450 bytes --]

On Sat, Aug 23, 2008 at 10:57:21AM +0200, Arnd Bergmann wrote:

> On Saturday 23 August 2008, Kevin Diggs wrote:
> > WARNING: externs should be avoided in .c files
> > #1137: FILE: powerpc/kernel/cpu/pll_if.c:369:
> > +       __asm__ __volatile__ (
> > 
> > ??? I don't know what this is?
> > 
> > The entire block is:
> > 
> >         __asm__ __volatile__ (
> >                 "addi %0,%3,-1\n"
> >                 "andc %1,%3,%0\n"
> >                 "cntlzw %1,%1\n"
> >                 "subfic %1,%1,31\n"
> >                 "cntlzw %0,%2\n":
> >                 "=r"(cntlz), "=r"(cnttz):
> >                 "r"(tmp), "b"(cnttz)
> >         );
> > 
> 
> It's a bug in checkpatch, your code is correct (although I would write
> asm volatile, not __asm__ __volatile__, and add \t after each \n).
> Can you explain why you need that inline assembly? All you do in there
> are arithmetic operations, so you should be able to express that using
> C, or at least using the helpers we already have.
> 
> Checkpatch thinks that what you wrote is a declaration for a function
> named __volatile__ returning a variable of type __asm__, and complains
> that this declaration belongs into a header file.

I wonder if checkpatch-maintainers read this list, so I put Andy
Whitcroft to CC.

   Wolfram

-- 
  Dipl.-Ing. Wolfram Sang | http://www.pengutronix.de
 Pengutronix - Linux Solutions for Science and Industry

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: checkpatch nits ...

[Andy Whitcroft]

On Wed, Aug 27, 2008 at 09:49:44AM +0200, Wolfram Sang wrote:
> On Sat, Aug 23, 2008 at 10:57:21AM +0200, Arnd Bergmann wrote:
> 
> > On Saturday 23 August 2008, Kevin Diggs wrote:
> > > WARNING: externs should be avoided in .c files
> > > #1137: FILE: powerpc/kernel/cpu/pll_if.c:369:
> > > +       __asm__ __volatile__ (
> > > 
> > > ??? I don't know what this is?
> > > 
> > > The entire block is:
> > > 
> > >         __asm__ __volatile__ (
> > >                 "addi %0,%3,-1\n"
> > >                 "andc %1,%3,%0\n"
> > >                 "cntlzw %1,%1\n"
> > >                 "subfic %1,%1,31\n"
> > >                 "cntlzw %0,%2\n":
> > >                 "=r"(cntlz), "=r"(cnttz):
> > >                 "r"(tmp), "b"(cnttz)
> > >         );
> > > 
> > 
> > It's a bug in checkpatch, your code is correct (although I would write
> > asm volatile, not __asm__ __volatile__, and add \t after each \n).
> > Can you explain why you need that inline assembly? All you do in there
> > are arithmetic operations, so you should be able to express that using
> > C, or at least using the helpers we already have.
> > 
> > Checkpatch thinks that what you wrote is a declaration for a function
> > named __volatile__ returning a variable of type __asm__, and complains
> > that this declaration belongs into a header file.
> 
> I wonder if checkpatch-maintainers read this list, so I put Andy
> Whitcroft to CC.

Thanks for the Cc:, even if you do read the lists you can easily miss
something of direct interest.

I actually think I saw a previous patch flow past showing this issue and
there appears to be a specific exception for __asm__ never being a type.
Cirtainly this fragment does not throw the error with the latest
checkpatch in my tree.  That fix appears to have gone in in 0.20.  Could
you check which version you have, the version number appears in the
usage message when checkpatch is invoked without parameters.  Also give
the version at the URL below a go, it represents the current development
copy.

  http://www.kernel.org/pub/linux/kernel/people/apw/checkpatch/checkpatch.pl-testing

Thanks for the report.

-apw