On Mon, Feb 08, 2016 at 01:54:04PM +1100, Paul Mackerras wrote:
> On Fri, Jan 29, 2016 at 04:23:57PM +1100, David Gibson wrote:
> > At the moment the hpte_removebolted callback in ppc_md returns void and
> > will BUG_ON() if the hpte it's asked to remove doesn't exist in the first
> > place.  This is awkward for the case of cleaning up a mapping which was
> > partially made before failing.
> > 
> > So, we add a return value to hpte_removebolted, and have it return ENOENT
> > in the case that the HPTE to remove didn't exist in the first place.
> > 
> > In the (sole) caller, we propagate errors in hpte_removebolted to its
> > caller to handle.  However, we handle ENOENT specially, continuing to
> > complete the unmapping over the specified range before returning the error
> > to the caller.
> > 
> > This means that htab_remove_mapping() will work sanely on a partially
> > present mapping, removing any HPTEs which are present, while also returning
> > ENOENT to its caller in case it's important there.
> > 
> > There are two callers of htab_remove_mapping():
> >    - In remove_section_mapping() we already WARN_ON() any error return,
> >      which is reasonable - in this case the mapping should be fully
> >      present
> >    - In vmemmap_remove_mapping() we BUG_ON() any error.  We change that to
> >      just a WARN_ON() in the case of ENOENT, since failing to remove a
> >      mapping that wasn't there in the first place probably shouldn't be
> >      fatal.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> 
> [snip]
> 
> > --- a/arch/powerpc/mm/hash_utils_64.c
> > +++ b/arch/powerpc/mm/hash_utils_64.c
> > @@ -269,6 +269,7 @@ int htab_remove_mapping(unsigned long vstart, unsigned long vend,
> >  {
> >  	unsigned long vaddr;
> >  	unsigned int step, shift;
> > +	int rc = 0;
> >  
> >  	shift = mmu_psize_defs[psize].shift;
> >  	step = 1 << shift;
> > @@ -276,10 +277,13 @@ int htab_remove_mapping(unsigned long vstart, unsigned long vend,
> >  	if (!ppc_md.hpte_removebolted)
> >  		return -ENODEV;
> >  
> > -	for (vaddr = vstart; vaddr < vend; vaddr += step)
> > -		ppc_md.hpte_removebolted(vaddr, psize, ssize);
> > +	for (vaddr = vstart; vaddr < vend; vaddr += step) {
> > +		rc = ppc_md.hpte_removebolted(vaddr, psize, ssize);
> > +		if ((rc < 0) && (rc != -ENOENT))
> > +			return rc;
> > +	}
> >  
> > -	return 0;
> > +	return rc;
> 
> This will return the rc from the last hpte_removebolted call, which
> might be 0 even if earlier calls had returned -ENOENT.  Or, if the
> last call fails with -ENOENT, this will return -ENOENT.  Is that
> exactly what you meant?  In the case where some calls to
> hpte_removebolted return -ENOENT, I would think we would want a
> consistent return value, which could be either 0 or -ENOENT, but it
> shouldn't depend on which specific calls fail with -ENOENT, in my
> opinion.

I agree.  The intention was that this returned -ENOENT iff any of the
individual calls did, but I messed up the logic; thanks for the catch.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson