linux-kernel - Re: [patch 03/36] Hexagon: Add bitops support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 17 Aug 2011 21:19 +0200
From:	Arnd Bergmann <arnd@...db.de>
To:	Richard Kuo <rkuo@...eaurora.org>
Cc:	linux-kernel@...r.kernel.org, linux-hexagon@...r.kernel.org
Subject: Re: [patch 03/36] Hexagon: Add bitops support

On Wednesday 17 August 2011 11:35:00 Richard Kuo wrote:

> +
> +#ifdef CONFIG_HEXAGON_BITOPS
> +#include <asm/bitops/ffz.h>
> +#include <asm/bitops/fls.h>
> +#include <asm/bitops/ffs.h>
> +#include <asm/bitops/__ffs.h>
> +#include <asm/bitops/__fls.h>
> +#else
> +#include <asm-generic/bitops/ffz.h>
> +#include <asm-generic/bitops/fls.h>
> +#include <asm-generic/bitops/ffs.h>
> +#include <asm-generic/bitops/__ffs.h>
> +#define __fls(x) (fls(x) - 1)
> +#endif

Is there a particular reason why this is configurable? Also, I see no reaons
to split these up into lots of files. Just put all of them into one file.

> +/*
> + * __ffs - find first bit in word.
> + * @word: The word to search
> + *
> + * Undefined if no bit exists, so code should check against 0 first.
> + *
> + * bits_per_long assumed to be 32
> + * numbering starts at 0 I think (instead of 1 like ffs)
> + */
> +static inline unsigned long __ffs(unsigned long word)
> +{
> +	int num;
> +
> +	asm volatile("%0 = ct0(%1);\n"
> +		: "=&r" (num)
> +		: "r" (word));
> +
> +	return num;
> +}

I think this should not be marked volatile: If the result is unused,
there is no need to compute the value either. Same for __fls, ffs,
ffz, and fls.

> +/*
> + * ffs - find first bit set
> + * @x: the word to search
> + *
> + * This is defined the same way as
> + * the libc and compiler builtin ffs routines, therefore
> + * differs in spirit from the above ffz (man ffs).
> + */
> +static inline int ffs(int x)
> +{

The type is normally 'long', not 'int'. They are obviously the
same length on 32 bit machines, but you might get bogus compiler
warnings from this. Same for ffz and fls.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/