linux-kernel - Re: [PATCH v5 20/27] cxl/extent: Process DCD events and realize region extents

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <672260877ccb7_483142942f@iweiny-mobl.notmuch>
Date: Wed, 30 Oct 2024 11:36:23 -0500
From: Ira Weiny <ira.weiny@...el.com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>, <ira.weiny@...el.com>
CC: Dave Jiang <dave.jiang@...el.com>, Fan Ni <fan.ni@...sung.com>, "Navneet
 Singh" <navneet.singh@...el.com>, Jonathan Corbet <corbet@....net>, "Andrew
 Morton" <akpm@...ux-foundation.org>, Dan Williams <dan.j.williams@...el.com>,
	Davidlohr Bueso <dave@...olabs.net>, Alison Schofield
	<alison.schofield@...el.com>, Vishal Verma <vishal.l.verma@...el.com>,
	<linux-cxl@...r.kernel.org>, <linux-doc@...r.kernel.org>,
	<nvdimm@...ts.linux.dev>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v5 20/27] cxl/extent: Process DCD events and realize
 region extents

Jonathan Cameron wrote:
> A few minor things inline from a fresh read.
> 
> Other than maybe a missing header, the others are all trivial
> and you can make your own minds up.
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@...ei.com>
> 

[snip]

> 
> > +static bool extents_contain(struct cxl_dax_region *cxlr_dax,
> > +			    struct cxl_endpoint_decoder *cxled,
> > +			    struct range *new_range)
> > +{
> > +	struct match_data md = {
> > +		.cxled = cxled,
> > +		.new_range = new_range,
> > +	};
> > +
> > +	struct device *extent_device __free(put_device)
> > +			= device_find_child(&cxlr_dax->dev, &md, match_contains);
> > +	if (!extent_device)
> > +		return false;
> > +
> > +	return true;
> trivial but could do.
> 
> 	return extent_device != NULL;

Sorry I tend to be more explicit...


[snip]

> 
> > +}
> 
> > +static int cxlr_rm_extent(struct device *dev, void *data)
> > +{
> > +	struct region_extent *region_extent = to_region_extent(dev);
> > +	struct range *region_hpa_range = data;
> > +
> > +	if (!region_extent)
> > +		return 0;
> > +
> > +	/*
> > +	 * Any extent which 'touches' the released range is removed.
> 
> Maybe single line comment syntax is fine here.

Ah...  yea this is an artifact of refactoring.  Later in the series the
comment becomes.

        /*
         * Any extent which 'touches' the released range is attempted to be
         * removed.
         */

So...

> 
> > +	 */
> > +	if (range_overlaps(region_hpa_range, &region_extent->hpa_range)) {
> > +		dev_dbg(dev, "Remove region extent HPA [range 0x%016llx-0x%016llx]\n",
> > +			region_extent->hpa_range.start, region_extent->hpa_range.end);
> > +		region_rm_extent(region_extent);
> > +	}
> > +	return 0;
> > +}
> 

[snip]

> > diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> > index 16e06b59d7f04762ca73a81740b0d6b2487301af..85b30a74a6fa5de1dd99c08c8318edd204e3e19d 100644
> > --- a/drivers/cxl/cxlmem.h
> > +++ b/drivers/cxl/cxlmem.h
> 
> Is the xarray header included in here already?
> If not it should be.

Looking around we have been lax in this behavior.  cxl.h does not explicitly
include xarray.h either.  I agree they both should after this.

Let me send a follow on patch to add it.

Ira

> 
> > @@ -506,6 +506,7 @@ static inline struct cxl_dev_state *mbox_to_cxlds(struct cxl_mailbox *cxl_mbox)
> >   * @pmem_perf: performance data entry matched to PMEM partition
> >   * @nr_dc_region: number of DC regions implemented in the memory device
> >   * @dc_region: array containing info about the DC regions