Re: [PATCH v4 25/28] cxl/region: Read existing extents on region creation

[Ira Weiny]

Jonathan Cameron wrote:
> On Mon, 07 Oct 2024 18:16:31 -0500
> ira.weiny@xxxxxxxxx wrote:
> 
> > From: Navneet Singh <navneet.singh@xxxxxxxxx>
> > 

[snip]

> > 
> One buglet, and a request for an error message.
> With those.
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>

Thanks.

> 
> > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> > index d66beec687a0..6b25d15403a3 100644
> > --- a/drivers/cxl/core/mbox.c
> > +++ b/drivers/cxl/core/mbox.c
> > @@ -1697,6 +1697,111 @@ int cxl_dev_dynamic_capacity_identify(struct cxl_memdev_state *mds)
> >  }
> >  EXPORT_SYMBOL_NS_GPL(cxl_dev_dynamic_capacity_identify, CXL);
> >  
> > +/* Return -EAGAIN if the extent list changes while reading */
> > +static int __cxl_process_extent_list(struct cxl_endpoint_decoder *cxled)
> > +{
> > +	u32 current_index, total_read, total_expected, initial_gen_num;
> > +	struct cxl_memdev_state *mds = cxled_to_mds(cxled);
> > +	struct cxl_mailbox *cxl_mbox = &mds->cxlds.cxl_mbox;
> > +	struct device *dev = mds->cxlds.dev;
> > +	struct cxl_mbox_cmd mbox_cmd;
> > +	u32 max_extent_count;
> > +	bool first = true;
> > +
> > +	struct cxl_mbox_get_extent_out *extents __free(kfree) =
> 
> __free(kvfree)

Yep fixed

> 
> > +				kvmalloc(cxl_mbox->payload_size, GFP_KERNEL);
> > +	if (!extents)
> > +		return -ENOMEM;
> 
> ...
> 
> 
> > +}
> 
> >  static void cxlr_dax_unregister(void *_cxlr_dax)
> >  {
> >  	struct cxl_dax_region *cxlr_dax = _cxlr_dax;
> > @@ -3224,6 +3233,9 @@ static int devm_cxl_add_dax_region(struct cxl_region *cxlr)
> >  	dev_dbg(&cxlr->dev, "%s: register %s\n", dev_name(dev->parent),
> >  		dev_name(dev));
> >  
> > +	if (cxlr->mode == CXL_REGION_DC)
> > +		cxlr_add_existing_extents(cxlr);
> 
> Whilst there isn't a whole lot we can do if this fails, I'd like an error
> print to indicate something odd is going on.  Probably pass any error
> up to here then print a message before carrying on.

Bubbled up an error and added some dev_err() calls.  I don't think they
need to be rate limited since regions are not created very often.

Ira

[snip]