Dave Jiang wrote:
>
>
> On 8/16/24 7:44 AM, ira.weiny@xxxxxxxxx wrote:
> > From: Navneet Singh <navneet.singh@xxxxxxxxx>
> >
[snip]
> >
> > Process DCD events and create region devices.
> >
> > Signed-off-by: Navneet Singh <navneet.singh@xxxxxxxxx>
> > Co-developed-by: Ira Weiny <ira.weiny@xxxxxxxxx>
> > Signed-off-by: Ira Weiny <ira.weiny@xxxxxxxxx>
>
> A few nits below, but in general
> Reviewed-by: Dave Jiang <dave.jiang@xxxxxxxxx>
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 = ®ion_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]
