Re: [PATCH v5 3/8] cxl/edac: Add CXL memory device patrol scrub control feature

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

On Mon, 19 May 2025 19:02:17 -0700
Alison Schofield <alison.schofield@intel.com> wrote:

> On Thu, May 15, 2025 at 12:59:19PM +0100, shiju.jose@huawei.com wrote:
> > From: Shiju Jose <shiju.jose@huawei.com>
> > 
> > CXL spec 3.2 section 8.2.10.9.11.1 describes the device patrol scrub
> > control feature. The device patrol scrub proactively locates and makes
> > corrections to errors in regular cycle.
> >   
> 
> snip
> 
> > +
> > +static int cxl_scrub_get_attrbs(struct cxl_patrol_scrub_context *cxl_ps_ctx,
> > +				u8 *cap, u16 *cycle, u8 *flags, u8 *min_cycle)
> > +{
> > +	struct cxl_mailbox *cxl_mbox;
> > +	u8 min_scrub_cycle = U8_MAX;
> > +	struct cxl_memdev *cxlmd;
> > +	int i, ret;
> > +
> > +	if (cxl_ps_ctx->cxlr) {
> > +		struct cxl_region *cxlr = cxl_ps_ctx->cxlr;
> > +		struct cxl_region_params *p = &cxlr->params;  
> 
> This function and the next, have a big if { } wrapper around
> cxlr existence. Can this logic be reversed -
> 
> ie, declare cxl_region and cxl_region_params in the header and
> then do something like - 
> 
> 	if (!cxl_ps_ctx->cxlr) {
> 		cxl_mbox = &cxl_ps_ctx->cxlmd->cxlds->cxl_mbox;
> 		return cxl_mem_scrub_get_attrbs(cxl_mbox, cap, cycle, flags, min_cycle);
> 	}
> 
> 	cxlr =  cxl_ps_ctx->cxlr;
> 	p = &cxlr->params;
> 
> Then all this code below can shift left.

See below - I'd factor out the region and no region cases because
then it will looks same as messier case below.

> 
> > +
> > +		struct rw_semaphore *region_lock __free(rwsem_read_release) =
> > +			rwsem_read_intr_acquire(&cxl_region_rwsem);
> > +		if (!region_lock)
> > +			return -EINTR;
> > +
> > +		for (i = 0; i < p->nr_targets; i++) {
> > +			struct cxl_endpoint_decoder *cxled = p->targets[i];
> > +
> > +			cxlmd = cxled_to_memdev(cxled);
> > +			cxl_mbox = &cxlmd->cxlds->cxl_mbox;
> > +			ret = cxl_mem_scrub_get_attrbs(cxl_mbox, cap, cycle,
> > +						       flags, min_cycle);
> > +			if (ret)
> > +				return ret;
> > +
> > +			if (min_cycle)
> > +				min_scrub_cycle =
> > +					min(*min_cycle, min_scrub_cycle);
> > +		}
> > +
> > +		if (min_cycle)
> > +			*min_cycle = min_scrub_cycle;
> > +
> > +		return 0;
> > +	}
> > +	cxl_mbox = &cxl_ps_ctx->cxlmd->cxlds->cxl_mbox;
> > +
> > +	return cxl_mem_scrub_get_attrbs(cxl_mbox, cap, cycle, flags, min_cycle);
> > +}
> > +
> > +static int cxl_scrub_set_attrbs(struct device *dev,
> > +				struct cxl_patrol_scrub_context *cxl_ps_ctx,
> > +				u8 cycle, u8 flags)
> > +{
> > +	struct cxl_scrub_wr_attrbs wr_attrbs;
> > +	struct cxl_mailbox *cxl_mbox;
> > +	struct cxl_memdev *cxlmd;
> > +	int ret, i;
> > +
> > +	wr_attrbs.scrub_cycle_hours = cycle;
> > +	wr_attrbs.scrub_flags = flags;
> > +
> > +	if (cxl_ps_ctx->cxlr) {
> > +		struct cxl_region *cxlr = cxl_ps_ctx->cxlr;
> > +		struct cxl_region_params *p = &cxlr->params;  
> 
> Similar to above function, but more work in the !cxlr case. Maybe a goto.
> 

A goto would be nasty. Given almost total lack of shared code
why not have
cxl_scrub_set_attrbs_region() and
cxl_scrub_set_attrbs_device() 

and this just becomes
	if (cxl_ps_cts->cxlr)
		return cxl_scrub_set_attrbs_region(dev, cxl_ps_ctx, cycle, flags);
	return cxl_scrub_set_attrbs_device(dev, cxl_ps_ctx, cycle, flags);

(or something along those lines anyway)

> 
> > +
> > +		struct rw_semaphore *region_lock __free(rwsem_read_release) =
> > +			rwsem_read_intr_acquire(&cxl_region_rwsem);
> > +		if (!region_lock)
> > +			return -EINTR;
> > +
> > +		for (i = 0; i < p->nr_targets; i++) {
> > +			struct cxl_endpoint_decoder *cxled = p->targets[i];
> > +
> > +			cxlmd = cxled_to_memdev(cxled);
> > +			cxl_mbox = &cxlmd->cxlds->cxl_mbox;
> > +			ret = cxl_set_feature(cxl_mbox, &CXL_FEAT_PATROL_SCRUB_UUID,
> > +					      cxl_ps_ctx->set_version, &wr_attrbs,
> > +					      sizeof(wr_attrbs),
> > +					      CXL_SET_FEAT_FLAG_DATA_SAVED_ACROSS_RESET,
> > +					      0, NULL);
> > +			if (ret)
> > +				return ret;
> > +
> > +			if (cycle != cxlmd->cur_scrub_cycle) {
> > +				if (cxlmd->cur_region_id != -1)
> > +					dev_info(dev,
> > +						 "Device scrub rate(%d hours) set by region%d rate overwritten by region%d scrub rate(%d hours)\n",
> > +						 cxlmd->cur_scrub_cycle,
> > +						 cxlmd->cur_region_id, cxlr->id,
> > +						 cycle);
> > +
> > +				cxlmd->cur_scrub_cycle = cycle;
> > +				cxlmd->cur_region_id = cxlr->id;
> > +			}
> > +		}
> > +
> > +		return 0;
> > +	}
> > +
> > +	cxlmd = cxl_ps_ctx->cxlmd;
> > +	cxl_mbox = &cxlmd->cxlds->cxl_mbox;
> > +	ret = cxl_set_feature(cxl_mbox, &CXL_FEAT_PATROL_SCRUB_UUID,
> > +			      cxl_ps_ctx->set_version, &wr_attrbs,
> > +			      sizeof(wr_attrbs),
> > +			      CXL_SET_FEAT_FLAG_DATA_SAVED_ACROSS_RESET, 0,
> > +			      NULL);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (cycle != cxlmd->cur_scrub_cycle) {
> > +		if (cxlmd->cur_region_id != -1)
> > +			dev_info(dev,
> > +				 "Device scrub rate(%d hours) set by region%d rate overwritten with device local scrub rate(%d hours)\n",
> > +				 cxlmd->cur_scrub_cycle, cxlmd->cur_region_id,
> > +				 cycle);
> > +
> > +		cxlmd->cur_scrub_cycle = cycle;
> > +		cxlmd->cur_region_id = -1;
> > +	}
> > +
> > +	return 0;
> > +}
> > +