linux-kernel - Re: [PATCH v3 06/25] cxl/mem: Read dynamic capacity configuration from the device

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: <66c7edbbbbccb_1719d2942d@iweiny-mobl.notmuch>
Date: Thu, 22 Aug 2024 21:02:35 -0500
From: Ira Weiny <ira.weiny@...el.com>
To: Fan Ni <nifan.cxl@...il.com>, Dave Jiang <dave.jiang@...el.com>
CC: <ira.weiny@...el.com>, Jonathan Cameron <Jonathan.Cameron@...wei.com>,
	Navneet Singh <navneet.singh@...el.com>, Chris Mason <clm@...com>, "Josef
 Bacik" <josef@...icpanda.com>, David Sterba <dsterba@...e.com>, Petr Mladek
	<pmladek@...e.com>, Steven Rostedt <rostedt@...dmis.org>, Andy Shevchenko
	<andriy.shevchenko@...ux.intel.com>, Rasmus Villemoes
	<linux@...musvillemoes.dk>, Sergey Senozhatsky <senozhatsky@...omium.org>,
	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-btrfs@...r.kernel.org>,
	<linux-cxl@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<linux-doc@...r.kernel.org>, <nvdimm@...ts.linux.dev>, "Li, Ming"
	<ming4.li@...el.com>
Subject: Re: [PATCH v3 06/25] cxl/mem: Read dynamic capacity configuration
 from the device

Fan Ni wrote:
> On Fri, Aug 16, 2024 at 02:45:47PM -0700, Dave Jiang wrote:

[snip]

> > > +
> > > +/**
> > > + * cxl_dev_dynamic_capacity_identify() - Reads the dynamic capacity
> > > + *					 information from the device.
> > > + * @mds: The memory device state
> > > + *
> > > + * Read Dynamic Capacity information from the device and populate the state
> > > + * structures for later use.
> > > + *
> > > + * Return: 0 if identify was executed successfully, -ERRNO on error.
> > > + */
> > > +int cxl_dev_dynamic_capacity_identify(struct cxl_memdev_state *mds)
> > > +{
> > > +	size_t dc_resp_size = mds->payload_size;
> > > +	struct device *dev = mds->cxlds.dev;
> > > +	u8 start_region, i;
> > > +
> > > +	for (i = 0; i < CXL_MAX_DC_REGION; i++)
> > > +		snprintf(mds->dc_region[i].name, CXL_DC_REGION_STRLEN, "<nil>");
> > > +
> > > +	if (!cxl_dcd_supported(mds)) {
> > > +		dev_dbg(dev, "DCD not supported\n");
> > > +		return 0;
> > > +	}
> > 
> > This should happen before you pre-format the name string? I would assume that if DCD is not supported then the dcd name sysfs attribs would be not be visible?
> > 

No this string is not used for sysfs.  It is used to label the dpa
resources...  That said in review I don't recall why it was necessary to
add the '<nil>' to them by default.  I'm actually going to remove that and
continue testing and if I recall where this was showing up I might add it
back in.

> > > +
> > > +	struct cxl_mbox_get_dc_config_out *dc_resp __free(kfree) =
> > > +					kvmalloc(dc_resp_size, GFP_KERNEL);
> > > +	if (!dc_resp)
> > > +		return -ENOMEM;
> > > +
> > > +	start_region = 0;
> > > +	do {
> > > +		int rc, j;
> > > +
> > > +		rc = cxl_get_dc_config(mds, start_region, dc_resp, dc_resp_size);
> > > +		if (rc < 0) {
> > > +			dev_dbg(dev, "Failed to get DC config: %d\n", rc);
> > > +			return rc;
> > > +		}
> > > +
> > > +		mds->nr_dc_region += rc;
> > > +
> > > +		if (mds->nr_dc_region < 1 || mds->nr_dc_region > CXL_MAX_DC_REGION) {
> > > +			dev_err(dev, "Invalid num of dynamic capacity regions %d\n",
> > > +				mds->nr_dc_region);
> > > +			return -EINVAL;
> > > +		}
> > > +
> > > +		for (i = start_region, j = 0; i < mds->nr_dc_region; i++, j++) {
> > 
> > This should be 'j < mds->nr_dc_region'? Otherwise if your start region say is '3' and you have '2' DC regions, you never enter the loop. Or does that not happen? I also wonder if you need to check if 'start_region + mds->nr_dc_region > CXL_MAX_DC_REGION'.
> > 
> That can not happen, start_region was updated to the number of regions
> has returned till now (not counting the current call), while
> nr_dc_region is the total number of regions returned till now (including
> the current call) as we update it above, so start_region should never be larger
> than nr_dc_region.

Yep.

> 
> > > +			rc = cxl_dc_save_region_info(mds, i, &dc_resp->region[j]);
> > > +			if (rc) {
> > > +				dev_dbg(dev, "Failed to save region info: %d\n", rc);
> 
> I am not sure why we sometimes use dev_err and sometimes we use dev_dbg
> here, if dcd is supported, error from getting dc configuration is an
> error to me.

We are trying to reduce the dev_err() use.  cxl_dc_save_region_info() has
dev_err() which is much more specific as to the error.  At worse this is
just redundant as a debug.

I'll remove it because the debug output is pretty verbose too.

Ira

> 
> Fan

[snip]