Re: [PATCH 01/12] perf/breakpoint: Split attribute parse and commit

[Frederic Weisbecker <frederic@kernel.org>]

On Thu, May 24, 2018 at 11:56:01AM +1000, Michael Ellerman wrote:
> Frederic Weisbecker <frederic@kernel.org> writes:
> 
> > diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
> > index 6e28d28..51320c2 100644
> > --- a/kernel/events/hw_breakpoint.c
> > +++ b/kernel/events/hw_breakpoint.c
> > @@ -424,19 +443,22 @@ static int validate_hw_breakpoint(struct perf_event *bp)
> >  
> >  int register_perf_hw_breakpoint(struct perf_event *bp)
> >  {
> > -	int ret;
> > +	struct arch_hw_breakpoint hw;
> > +	int err;
> >  
> > -	ret = reserve_bp_slot(bp);
> > -	if (ret)
> > -		return ret;
> > +	err = reserve_bp_slot(bp);
> > +	if (err)
> > +		return err;
> >  
> > -	ret = validate_hw_breakpoint(bp);
> > -
> > -	/* if arch_validate_hwbkpt_settings() fails then release bp slot */
> > -	if (ret)
> > +	err = hw_breakpoint_parse(bp, &bp->attr, &hw);
> 
> Is there a good reason we pass bp and bp->attr? (I assume so)
> 
> That added to the confusion in the existing code I think.

Yes, on breakpoint creation (which is the above function) it's not needed
but breakpoint modification wants it as we need to pass the attr that are
to be validated, and those are not yet copied to the breakpoint at this
stage. This happens in the end of the series.

Thanks.