linux-kernel - Re: [PATCH v3 18/25] 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: <66c7f998ca413_1719d2947a@iweiny-mobl.notmuch>
Date: Thu, 22 Aug 2024 21:53:12 -0500
From: Ira Weiny <ira.weiny@...el.com>
To: Dave Jiang <dave.jiang@...el.com>, <ira.weiny@...el.com>, Fan Ni
	<fan.ni@...sung.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>
CC: 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>
Subject: Re: [PATCH v3 18/25] cxl/extent: Process DCD events and realize
 region extents

Dave Jiang wrote:
> 
> 
> On 8/16/24 7:44 AM, ira.weiny@...el.com wrote:
> > From: Navneet Singh <navneet.singh@...el.com>
> > 

[snip]

> > 
> > Process DCD events and create region devices.
> > 
> > Signed-off-by: Navneet Singh <navneet.singh@...el.com>
> > Co-developed-by: Ira Weiny <ira.weiny@...el.com>
> > Signed-off-by: Ira Weiny <ira.weiny@...el.com>
> 
> A few nits below, but in general
> Reviewed-by: Dave Jiang <dave.jiang@...el.com>

Thanks.

> > +
> > +static int online_region_extent(struct region_extent *region_extent)
> > +{
> > +	struct cxl_dax_region *cxlr_dax = region_extent->cxlr_dax;
> > +	struct device *dev;
> > +	int rc;
> > +
> > +	dev = &region_extent->dev;
> 
> Nit. You can move this up to when you declare 'dev'.

[done.]

[snip]

> > +
> > +static int cxl_add_pending(struct cxl_memdev_state *mds)
> > +{
> > +	struct device *dev = mds->cxlds.dev;
> > +	struct cxl_extent *extent;
> > +	unsigned long index;
> > +	unsigned long cnt = 0;
> reverse xmas tree

yep.
[done.]


[snip]

> > +
> > +static int handle_add_event(struct cxl_memdev_state *mds,
> > +			    struct cxl_event_dcd *event)
> > +{
> > +	struct cxl_extent *tmp = kzalloc(sizeof(*tmp), GFP_KERNEL);
> for readability I would use *extent instead of *tmp

sure.
[done.]


[snip]

> >  
> > diff --git a/include/linux/cxl-event.h b/include/linux/cxl-event.h
> > index 0bea1afbd747..eeda8059d81a 100644
> > --- a/include/linux/cxl-event.h
> > +++ b/include/linux/cxl-event.h
> > @@ -96,11 +96,43 @@ struct cxl_event_mem_module {
> >  	u8 reserved[0x3d];
> Previous code, but 61 would be better than 0x3d to be consistent with rest of cxl code

:-(

I get the rest of the code argument.  However, the specification uses hex
for the number of bytes in the definitions.  For this reason I prefer the
use of hex here so that one can better match the code to the spec.

> 
> >  } __packed;
> >  
> > +/*
> > + * CXL rev 3.1 section 8.2.9.2.1.6; Table 8-51
> > + */
> > +#define CXL_EXTENT_TAG_LEN 0x10
> > +struct cxl_extent {
> > +	__le64 start_dpa;
> > +	__le64 length;
> > +	u8 tag[CXL_EXTENT_TAG_LEN];
> > +	__le16 shared_extn_seq;
> > +	u8 reserved[0x6];
> 
> Why not just 6? In general I find it odd that this header uses hex for
> array indexing when the rest of the cxl code uses decimal. 

I was just directly matching the spec.

> 
> > +} __packed;
> > +
> > +/*
> > + * Dynamic Capacity Event Record
> > + * CXL rev 3.1 section 8.2.9.2.1.6; Table 8-50
> > + */
> > +#define CXL_DCD_EVENT_MORE			BIT(0)
> > +struct cxl_event_dcd {
> > +	struct cxl_event_record_hdr hdr;
> > +	u8 event_type;
> > +	u8 validity_flags;
> > +	__le16 host_id;
> > +	u8 region_index;
> > +	u8 flags;
> > +	u8 reserved1[0x2];
> 
> also here, 2?

Same...  I know it is odd when the hex string == the decimal string.

> 
> > +	struct cxl_extent extent;
> > +	u8 reserved2[0x18];
> 
> 24?

same.

Ira

[snip]