Re: [PATCH v3 2/2] cxl/region: Remove a soft reserved resource at region teardown

[Alison Schofield <alison.schofield@intel.com>]

On Tue, Aug 29, 2023 at 02:17:02PM +0100, Jonathan Cameron wrote:
> 
> Hi Alison,
> 
> > > > +	/*
> > > > +	 * No alignment. Attempt a 3-way split that removes the part of
> > > > +	 * the resource the region occupied, and then creates new soft
> > > > +	 * reserved resources for the leading and trailing addr space.
> > > > +	 * adjust_resource() will stop the attempt if there are any
> > > > +	 * child resources.
> > > > +	 */
> > > > +
> > > > +	/* Save the original soft reserved resource params before adjusting */
> > > > +	new_start = soft->start;
> > > > +	new_end = soft->end;
> > > > +	parent = soft->parent;
> > > > +	flags = soft->flags;
> > > > +
> > > > +	rc = adjust_resource(soft, start, end - start);
> > > > +	if (rc) {
> > > > +		dev_dbg(&cxlr->dev,
> > > > +			"cannot adjust soft reserved resource %pr\n", soft);
> > > > +		return;
> > > > +	}
> > > > +	rc = remove_resource(soft);
> > > > +	if (rc)
> > > > +		dev_warn(&cxlr->dev,
> > > > +			 "cannot remove soft reserved resource %pr\n", soft);  
> > > 
> > > I'd like a comment on this first bit.  Why adjust a resource only to throw it
> > > away?  
> > 
> > I intended to give the big picture in the block comment above. Scroll up
> > and see /* No alignment... 
> > 
> > Did you overlook that or do you want more explanation or something else?
> 
> Overlooked it as you guessed.  Makes sense though it's ugly that we need
> that comment.  Maybe worth explicitly checking that there are no children
> rather than using a side effect of adjust_resource()?
> Might need a new generic resource_has_children() check or similar.
> 
> Jonathan

Hi Jonathan,
Thanks - agree on the explicit child check.

However, I'm reworking this Soft Reserved handling based on a chat with
Dan. New approach is to not have the CXL intersecting soft reserved
resources in iomem_resource tree. Only move them there if CXL region
assembly fails and we want to make them availabe to DAX directly.
Just wrapping my head around it.

So, both these patches go away.

Alison


> > 
> > Thanks,
> > Alison
> > 
> > > 
> > >   
> > > > +
> > > > +	rc = add_soft_reserved(parent, new_start, start - new_start, flags);
> > > > +	if (rc)
> > > > +		dev_warn(&cxlr->dev,
> > > > +			 "cannot add new soft reserved resource at %pa\n",
> > > > +			 &new_start);
> > > > +
> > > > +	rc = add_soft_reserved(parent, end + 1, new_end - end, flags);
> > > > +	if (rc)
> > > > +		dev_warn(&cxlr->dev,
> > > > +			 "cannot add new soft reserved resource at %pa + 1\n",
> > > > +			 &end);
> > > > +}
> > > > +
> > > >  static void cxl_region_iomem_release(struct cxl_region *cxlr)
> > > >  {
> > > >  	struct cxl_region_params *p = &cxlr->params;
> > > > +	struct resource *parent, *res = p->res;
> > > > +	resource_size_t start, end;
> > > >  
> > > >  	if (device_is_registered(&cxlr->dev))
> > > >  		lockdep_assert_held_write(&cxl_region_rwsem);
> > > > -	if (p->res) {
> > > > -		/*
> > > > -		 * Autodiscovered regions may not have been able to insert their
> > > > -		 * resource.
> > > > -		 */
> > > > -		if (p->res->parent)
> > > > -			remove_resource(p->res);
> > > > -		kfree(p->res);
> > > > -		p->res = NULL;
> > > > +	if (!res)
> > > > +		return;
> > > > +	/*
> > > > +	 * Autodiscovered regions may not have been able to insert their
> > > > +	 * resource. If a Soft Reserved parent resource exists, try to
> > > > +	 * remove that also.
> > > > +	 */
> > > > +	if (p->res->parent) {
> > > > +		parent = p->res->parent;
> > > > +		start = p->res->start;
> > > > +		end = p->res->end;
> > > > +		remove_resource(p->res);
> > > > +		if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags) &&
> > > > +		    parent->desc == IORES_DESC_SOFT_RESERVED)
> > > > +			remove_soft_reserved(cxlr, parent, start, end);
> > > >  	}
> > > > +
> > > > +	kfree(p->res);
> > > > +	p->res = NULL;
> > > >  }
> > > >  
> > > >  static int free_hpa(struct cxl_region *cxlr)  
> > >   
>