Re: [PATCH 8/8] cxl/memfeature: Add CXL memory device memory sparing control feature

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

On Fri, Mar 07, 2025 at 09:11:37AM +0800, Jonathan Cameron wrote:
> On Thu, 27 Feb 2025 22:38:15 +0000
> <shiju.jose@huawei.com> wrote:
> 
> > From: Shiju Jose <shiju.jose@huawei.com>
> > 
snip

> Similar comment to earlier on maybe using single line comments
> in more places rather than multiline.  Perhaps worth doing
> that if you are respinning for other reasons.

Following on Jonathan's feedback, a couple of things-

- Within the CXL subsystem (maybe it's kernel wide) there is a
style or custom, that comments that only need to occupy a single
line only use a single line. This set should follow that. When code
is styled uniformally it is easier to read.

- This next thing I recognize because I have a bad habit of doing
it myself. Narrating! Some of these (should be single line) comments
are needlessly narrating the code. A comment is useful if it explains
something not obvious, but when we have descriptive function names and
variables, less commentary is needed.

see below...

> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> > +static int cxl_mem_do_sparing_op(struct device *dev,
> > +				 struct cxl_mem_sparing_context *cxl_sparing_ctx,
> > +				 struct cxl_memdev_sparing_params *rd_params)
> > +{
> > +	struct cxl_memdev *cxlmd = cxl_sparing_ctx->cxlmd;
> > +	struct cxl_memdev_sparing_in_payload sparing_pi;
> > +	struct cxl_event_dram *rec = NULL;
> > +	u16 validity_flags = 0;
> > +
> > +	if (!rd_params->cap_safe_when_in_use) {
> > +		/*
> > +		 * Memory to repair must be offline
> > +		 */
> > +		if (cxl_are_decoders_committed(cxlmd))
> > +			return -EBUSY;
> > +		/*
> > +		 * offline, so good for repair
> > +		 */
> More places as below where a single line comment would be fine
> and make a reader scroll a bit less.

This got cut off, but I think the code can tell the story without
narration. (per my other patch feedback maybe you will rename this
something like is_memdev_memory_offline())?

snip

> > +	/*
> > +	 * Read CXL device's sparing capabilities.
> a below.
> > +	 */
> > +	ret = cxl_mem_sparing_get_attrs(cxl_sparing_ctx, &rd_params);

Great name, no clarifying comment needed.

I'm not looking through them all. I'll leave that to you. But I
appreciate you looking and updating to single line and removing
the comment entirely where needless. It helps keep the code base
uniform in style which makes reading it easier.

Thanks Shiju :)


> > +	if (ret)
> > +		return ret;
> > +
> > +	/*
> > +	 * Set default value for persist_mode.
> > +	 */
> 
> If respining some of these comments don't need to be multiline.
> 
>