Re: [RFC] 4xx hardware watchpoint support

[Christoph Hellwig <hch@lst.de>]

On Tue, Jul 22, 2008 at 10:47:58PM -0300, Luis Machado wrote:
> +void do_dabr(struct pt_regs *regs, unsigned long address,
> +		    unsigned long error_code)
> +{
> +	siginfo_t info;
> +
> +#if !(defined(CONFIG_4xx) || defined(CONFIG_BOOKE))
> +	if (notify_die(DIE_DABR_MATCH, "dabr_match", regs, error_code,
> +			11, SIGSEGV) == NOTIFY_STOP)
> +		return;
> +
> +	if (debugger_dabr_match(regs))
> +		return;
> +
> +	/* Clear the DAC and struct entries.  One shot trigger.  */
> +#else /* (defined(CONFIG_4xx) || defined(CONFIG_BOOKE)) */
> +	mtspr(SPRN_DBCR0, mfspr(SPRN_DBCR0) & ~(DBSR_DAC1R | DBSR_DAC1W
> +							| DBCR0_IDM));
> +#endif

Some comment, first the above negate conditional
looks rather ugly, I'd rather do a

#if defined(CONFIG_4xx) || defined(CONFIG_BOOKE)
	dbcr0 case
#else
	dabr case
#endif

second I wonder why we have the notify_die only for one case, that seems
rather odd.  Looking further the notify_die is even more odd because
DIE_DABR_MATCH doesn't actually appear anywhere else in the kernel.
I'd suggest simply removing it.

> +	/* For processors using DABR (i.e. 970), the bottom 3 bits are flags.
> +	   It was assumed, on previous implementations, that 3 bits were
> +	   passed together with the data address, fitting the design of the
> +	   DABR register, as follows:
> +
> +	   bit 0: Read flag
> +	   bit 1: Write flag
> +	   bit 2: Breakpoint translation
> +
> +	   Thus, we use them here as so.  */

Can you redo this in the normal Linux comment style, ala:

	/*
	 * For processors using DABR (i.e. 970), the bottom 3 bits are flags.
	 *  It was assumed, on previous implementations, that 3 bits were
	 *  passed together with the data address, fitting the design of the
	 *  DABR register, as follows:
	 *
	 *  bit 0: Read flag
	 *  bit 1: Write flag
	 *  bit 2: Breakpoint translation
	 *
	 *  Thus, we use them here as so.
	 */

and similar in few other places.