Re: [PATCH v2 08/30] Add sparc-specific parity functions

[Zeng Zhaoxiu]

å 2016å04æ07æ 02:44, Sam Ravnborg åé:
> Hi Zeng.
>
> > Use runtime patching for sparc64, lifted from hweight
> No errors found in patch - but a few comments.
> In general patch looks good.

Thanks. Sparc, powerpc, and x86 are all new to me.

> > +++ b/arch/sparc/include/asm/bitops_64.h
> > @@ -47,6 +47,24 @@ unsigned int __arch_hweight16(unsigned int w);
> >   unsigned int __arch_hweight8(unsigned int w);
> >   
> >   #include <asm-generic/bitops/const_hweight.h>
> > +
> > +/*
> > + * parityN: returns the parity of a N-bit word,
> > + * i.e. the number of 1-bits in w modulo 2.
> > + */
> > +
> > +static inline unsigned int __arch_parity4(unsigned int w)
> > +{
> > +	w &= 0xf;
> > +	return (0x6996 >> w) & 1;
> > +}
> As Josef already said - this constant should have a name.
> PARITY_BIT
> ?

Maybe PARITY_MAGIC?

> > +++ b/arch/sparc/kernel/sparc_ksyms_64.c
> > @@ -45,6 +45,12 @@ EXPORT_SYMBOL(__arch_hweight16);
> >   EXPORT_SYMBOL(__arch_hweight32);
> >   EXPORT_SYMBOL(__arch_hweight64);
> >   
> > +/* from parity.S */
> > +EXPORT_SYMBOL(__arch_parity8);
> > +EXPORT_SYMBOL(__arch_parity16);
> > +EXPORT_SYMBOL(__arch_parity32);
> > +EXPORT_SYMBOL(__arch_parity64);
> Did you compile this?
> I wonder if bitops_64.h is indirectly included.

Yes.

> > index 0000000..b1945e3
> > --- /dev/null
> > +++ b/arch/sparc/lib/parity.S
> > @@ -0,0 +1,93 @@
> > +#include <linux/linkage.h>
> > +
> > +	.text
> > +	.align	32
> > +
> > +ENTRY(__arch_parity8)
> > +	srl		%o0, 4, %g1
> > +	xor		%o0, %g1, %o0
> > +	and		%o0, 0xf, %o0
> > +	sethi		%hi(0x6996), %g1
> > +	or		%g1, %lo(0x6996), %g1
> > +	srl		%g1, %o0, %o0
> > +	retl
> > +	 and		%o0, 1, %o0
> > +ENDPROC(__arch_parity8)
> I know the level of comments in hweight is equal to none.
> But please do not follow this bad example.
> At least for each function a one-liner of the C code.
> And in the top of the file maybe two lines that the functions
> are patched at run-time if the processor has popc available.
>
>
> 	Sam

OK. I will try, but my english is very poor! :-)