Re: [PATCH v7] cxl/region: check interleave capability

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

On Tue, Jun 11, 2024 at 09:07:09PM -0700, Dan Williams wrote:
> Yao Xingtao wrote:
> > Since interleave capability is not verified, if the interleave
> > capability of a target does not match the region need, committing decoder
> > should have failed at the device end.
> > 
> > In order to checkout this error as quickly as possible, driver needs
> > to check the interleave capability of target during attaching it to
> > region.
> > 
> > Per CXL specification r3.1(8.2.4.20.1 CXL HDM Decoder Capability Register),
> > bits 11 and 12 indicate the capability to establish interleaving in 3, 6,
> > 12 and 16 ways. If these bits are not set, the target cannot be attached to
> > a region utilizing such interleave ways.
> > 
> > Additionally, bits 8 and 9 represent the capability of the bits used for
> > interleaving in the address, Linux tracks this in the cxl_port
> > interleave_mask.
> > 
> > Per CXL specification r3.1(8.2.4.20.13 Decoder Protection):
> >   eIW means encoded Interleave Ways.
> >   eIG means encoded Interleave Granularity.
> > 
> >   in HPA:
> >   if eIW is 0 or 8 (interleave ways: 1, 3), all the bits of HPA are used,
> >   the interleave bits are none, the following check is ignored.
> > 
> >   if eIW is less than 8 (interleave ways: 2, 4, 8, 16), the interleave bits
> >   start at bit position eIG + 8 and end at eIG + eIW + 8 - 1.
> > 
> >   if eIW is greater than 8 (interleave ways: 6, 12), the interleave bits
> >   start at bit position eIG + 8 and end at eIG + eIW - 1.
> > 
> >   if the interleave mask is insufficient to cover the required interleave
> >   bits, the target cannot be attached to the region.
> > 
> > Fixes: 384e624bb211 ("cxl/region: Attach endpoint decoders")
> > Signed-off-by: Yao Xingtao <yaoxt.fnst@fujitsu.com>
> > Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> [..]
> > diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> > index 36cee9c30ceb..7fe617122d33 100644
> > --- a/drivers/cxl/cxlmem.h
> > +++ b/drivers/cxl/cxlmem.h
> > @@ -848,11 +848,21 @@ static inline void cxl_mem_active_dec(void)
> >  
> >  int cxl_mem_sanitize(struct cxl_memdev *cxlmd, u16 cmd);
> >  
> > +/*
> 
> Minor detail that can come in a follow-on patch is that this needs to
> be:
> 
> /**
> 
> ...in order for the kernel-doc system to autoformat it like it does
> other 'struct' documentation:
> 
> https://docs.kernel.org/driver-api/cxl/memory-devices.html
> 
> However, the reason it needs to be a follow-on patch is that this file
> is not currently included for parsing and needs something like this:

We'll need another revision of this patch to address the cxl-test module
dependencies so how about adding the "/**" in the next revision.

I'm suggesting this because cxl_mem.h contains other kernel doc
comments that are not being picked up because cxl_mem.h is missing
in Documentation/driver-api/cxl/memory-devices.rst. There also seem
to be other ommissions when compared with the kernel doc notations
in drivers/cxl/ :  core/cdat.c, core/hdm.c. I say 'seems' because
I guess it could be intentional.

Can Yao add the kernel doc notation in the next rev of this patch
and then come back soon and sync memory-devices.rst for all of
drivers/cxl/ ?

-- Alison

> 
> diff --git a/Documentation/driver-api/cxl/memory-devices.rst b/Documentation/driver-api/cxl/memory-devices.rst
> index 5149ecdc53c7..e33ee67ac1a2 100644
> --- a/Documentation/driver-api/cxl/memory-devices.rst
> +++ b/Documentation/driver-api/cxl/memory-devices.rst
> @@ -325,6 +325,9 @@ CXL Memory Device
>  .. kernel-doc:: drivers/cxl/pci.c
>     :internal:
>  
> +.. kernel-doc:: drivers/cxl/cxlmem.h
> +   :internal:
> +
>  .. kernel-doc:: drivers/cxl/mem.c
>     :doc: cxl mem
>