Re: [Patch 00/12] Hardware Breakpoint interfaces

["K.Prasad" <prasad@linux.vnet.ibm.com>]

On Fri, May 15, 2009 at 01:38:29AM +0530, K.Prasad wrote:
> On Thu, May 14, 2009 at 04:02:16PM -0400, Alan Stern wrote:
> > On Wed, 13 May 2009, K.Prasad wrote:
> > 
> > > Hi Alan,
> > > 	Please find a new set of patches with the following changes.
> > > I am eager to know if you think that they are ready for submission towards
> > > upstream acceptance.
> > 
> > This is looking pretty good.  Only one thing caught my eye: 
> > switch_to_thread_hw_breakpoint and switch_to_none_hw_breakpoint do 
> > nothing but call the corresponding arch-specific routines.  You might 
> > as well eliminate them and have the callers invoke the arch-specific 
> > routines instead.
> > 
> > Alan Stern
> >
> 
> Sure. I will eliminate the above two functions and also remove the msleep()
> call in ftrace startup selftest code path.
> 
> With these two changes I plan to submit the code for -tip acceptance
> directly.
>

There is another change that I wanted to make in arch_store_info()
[which I realised while responding to comments on another list]. I think
it will be much safer to add a check to see if symbol_name is passed
when user-space breakpoints are requested and return -EINVAL if done so.

This will help avoid any confusion at the user, who can assume that
symbol names can be resolved, and specifying address is not required.

Here's how the modified arch_store_info() would look like:

/*
 * Store a breakpoint's encoded address, length, and type.
 */
static int arch_store_info(struct hw_breakpoint *bp, struct task_struct *tsk)
{
	/*
	 * User-space requests will always have the address field populated.
	 * Symbol names from user-space are rejected
	 */
	if (tsk && bp->info.name)
		return -EINVAL;
	/*
	 * For kernel-addresses, either the address or symbol name can be
	 * specified.
	 */
	if (bp->info.name)
		bp->info.address = (unsigned long)
					kallsyms_lookup_name(bp->info.name);
	if (bp->info.address)
		return 0;
	return -EINVAL;
}

Thanks,
K.Prasad