Re: [PATCH 1/12] mips: PMC MSP71xx core platform

[Ralf Baechle <ralf@linux-mips.org>]

On Thu, Jun 14, 2007 at 03:54:47PM -0600, Marc St-Jean wrote:

General comment all the first ~ 2900 of your patch is that it's rather
heavy on #ifdef.  #ifdef is a nasty construct that has a tendency to hard
to read code which in trun results in bugs.  Or a test compile fails to
find real issues in the code because it's hidden in a dead #if construct.

> diff --git a/include/asm-mips/pmc-sierra/msp71xx/msp_regops.h b/include/asm-mips/pmc-sierra/msp71xx/msp_regops.h
> new file mode 100644
> index 0000000..60a5a38
> --- /dev/null
> +++ b/include/asm-mips/pmc-sierra/msp71xx/msp_regops.h
> @@ -0,0 +1,236 @@
> +/*
> + * SMP/VPE-safe functions to access "registers" (see note).
> + *
> + * NOTES:
> +* - These macros use ll/sc instructions, so it is your responsibility to
> + * ensure these are available on your platform before including this file.
> + * - The MIPS32 spec states that ll/sc results are undefined for uncached
> + * accesses. This means they can't be used on HW registers accessed
> + * through kseg1. Code which requires these macros for this purpose must
> + * front-end the registers with cached memory "registers" and have a single
> + * thread update the actual HW registers.

You basically betting on undefined behaviour of the architecture.  I did
indeed verify this specific case with a processor design guy and the answer
was a "should be ok".  That is I heared people speak in a more convincing
tone already ;-)

A SC with an uncached address on some other MIPS processors will always fail.
So this isn't just a theoretical footnote in a manual.

The way things are implemented LL/SC I am certain that uncached LL/SC will
fail on any MIPS multiprocessor system.  Fortunately while SMTC pretends
to be multiprocessor it's really just a single core, which saves your day.

> + * - A maximum of 2k of code can be inserted between ll and sc. Every
> + * memory accesses between the instructions will increase the chance of
> + * sc failing and having to loop.

Any memory access between LL/SC makes the LL/SC sequence invalid that is
it will have undefined effects.

> + * - When using custom_read_reg32/custom_write_reg32 only perform the
> + * necessary logical operations on the register value in between these
> + * two calls. All other logic should be performed before the first call.
> +  * - There is a bug on the R10000 chips which has a workaround. If you
> + * are affected by this bug, make sure to define the symbol 'R10000_LLSC_WAR'
> + * to be non-zero.  If you are using this header from within linux, you may
> + * include <asm/war.h> before including this file to have this defined
> + * appropriately for you.

> +#ifndef __ASM_REGOPS_H__
> +#define __ASM_REGOPS_H__
> +
> +#include <linux/types.h>
> +
> +#include <asm/war.h>
> +
> +#ifndef R10000_LLSC_WAR
> +#define R10000_LLSC_WAR 0
> +#endif

This symbol is supposed to be defined by <asm/war.h> only.  Anyway, this
#ifndef will never be true because you already include <asm/war.h>, so
this is dead code.

> +#if R10000_LLSC_WAR == 1
> +#define __beqz	"beqzl	"
> +#else
> +#define __beqz	"beqz	"
> +#endif
> +
> +#ifndef _LINUX_TYPES_H
> +typedef unsigned int u32;
> +#endif

Redefining a stanard Linux type is a no-no as is relying on include
wrapper symbols like _LINUX_TYPES_H.  Anyway, this #ifndef will never
be true because you already include <linux/types.h>, so this is dead code.

> +static inline u32 read_reg32(volatile u32 *const addr,
> +				u32 const mask)
> +{
> +	u32 temp;
> +
> +	__asm__ __volatile__(
> +	"	.set	push				\n"
> +	"	.set	noreorder			\n"
> +	"	lw	%0, %1		# read		\n"
> +	"	and	%0, %2		# mask		\n"
> +	"	.set	pop				\n"
> +	: "=&r" (temp)
> +	: "m" (*addr), "ir" (mask));
> +
> +	return temp;
> +}

No need for inline assembler here; plain C can achieve the same.  Or just
use a standard Linux function such as readl() or ioread32() or similar.

> +/*
> + * For special strange cases only:
> + *
> + * If you need custom processing within a ll/sc loop, use the following macros
> + * VERY CAREFULLY:
> + *
> + *   u32 tmp;				<-- Define a variable to hold the data
> + *
> + *   custom_read_reg32(address, tmp);	<-- Reads the address and put the value
> + *						in the 'tmp' variable given
> + *
> + *	From here on out, you are (basicly) atomic, so don't do anything too
> + *	fancy!
> + *	Also, this code may loop if the end of this block fails to write
> + *	everything back safely due do the other CPU, so do NOT do anything
> + *	with side-effects!
> + *
> + *   custom_write_reg32(address, tmp);	<-- Writes back 'tmp' safely.
> + */
> +#define custom_read_reg32(address, tmp)				\
> +	__asm__ __volatile__(					\
> +	"	.set	push				\n"	\
> +	"	.set	mips3				\n"	\
> +	"1:	ll	%0, %1	#custom_read_reg32	\n"	\
> +	"	.set	pop				\n"	\
> +	: "=r" (tmp), "=m" (*address)				\
> +	: "m" (*address))
> +
> +#define custom_write_reg32(address, tmp)			\
> +	__asm__ __volatile__(					\
> +	"	.set	push				\n"	\
> +	"	.set	mips3				\n"	\
> +	"	sc	%0, %1	#custom_write_reg32	\n"	\
> +	"	"__beqz"%0, 1b				\n"	\
> +	"	nop					\n"	\
> +	"	.set	pop				\n"	\
> +	: "=&r" (tmp), "=m" (*address)				\
> +	: "0" (tmp), "m" (*address))

These two are *really* fragile stuff.  Modern gcc rearranges code in
amazing ways, so you might end up with other loads or stores being moved
into the ll/sc sequence or the 1: label of another inline assembler
construct being taken as the destination of the branch.  So I would
suggest to safely store the two function in a nice yellow barrel ;-)

General suggestion, you can make about every access atomic if you do
something like

#include <linux/modules.h>
#include <linux/spinlocks.h>

DEFINE_SPINLOCK(register_lock);
EXPORT_SYMBOL(register_lock);

static inline void set_value_reg32(u32 *const addr,
                                       u32 const mask,
                                       u32 const value)
{
	unsigned long flags;
	u32 bits;

	spinlock_irqsave(&register_lock, flags);
	bits = readl(addr);
	bits &= mask;
	bits |= value;
	writel(bits, addr);
}

Maybe slower but definately more portable and not waiting before some
CPU designer screws your code by accident :-)

  Ralf