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

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

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
> 
> 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)  
> >