Re: [PATCH 1/5] cxl/mem : Read Dynamic capacity configuration from the device

[Ira Weiny <ira.weiny@intel.com>]

Dave Jiang wrote:
> 
> 
> On 6/14/23 12:16, ira.weiny@intel.com wrote:
> > From: Navneet Singh <navneet.singh@intel.com>
> > 
> > Read the Dynamic capacity configuration and store dynamic capacity region
> > information in the device state which driver will use to map into the HDM
> > ranges.
> > 
> > Implement Get Dynamic Capacity Configuration (opcode 4800h) mailbox
> > command as specified in CXL 3.0 spec section 8.2.9.8.9.1.
> > 
> > Signed-off-by: Navneet Singh <navneet.singh@intel.com>
> > 
> > ---
> > [iweiny: ensure all mds->dc_region's are named]
> > ---
> >   drivers/cxl/core/mbox.c | 190 ++++++++++++++++++++++++++++++++++++++++++++++--
> >   drivers/cxl/cxlmem.h    |  70 +++++++++++++++++-
> >   drivers/cxl/pci.c       |   4 +
> >   3 files changed, 256 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> > index 3ca0bf12c55f..c5b696737c87 100644
> > --- a/drivers/cxl/core/mbox.c
> > +++ b/drivers/cxl/core/mbox.c
> > @@ -111,6 +111,37 @@ static u8 security_command_sets[] = {
> >   	0x46, /* Security Passthrough */
> >   };
> >   
> > +static bool cxl_is_dcd_command(u16 opcode)
> > +{
> > +#define CXL_MBOX_OP_DCD_CMDS 0x48
> 
> Move this to cxlmem.h?

This is only ever used in mbox.c.  So I left the scope restricted for now.
If modules need it in the future we can lift it.  Also
cxl_is_poison_command() is also this way.

> 
> > +
> > +	if ((opcode >> 8) == CXL_MBOX_OP_DCD_CMDS)
> > +		return true;
> > +
> > +	return false;
> 
> I think you simplify by:
> 
> return (opcode >> 8) == CXL_MBOX_OP_DCD_CMDS;

Good catch.  I'll clean it up.

[snip]

> > @@ -1121,13 +1289,23 @@ int cxl_mem_create_range_info(struct cxl_memdev_state *mds)
> >   	}
> >   
> >   	cxlds->dpa_res =
> > -		(struct resource)DEFINE_RES_MEM(0, mds->total_bytes);
> > +		(struct resource)DEFINE_RES_MEM(0, mds->total_capacity);
> > +
> > +	for (int i = 0; i < CXL_MAX_DC_REGION; i++) {
> > +		struct cxl_dc_region_info *dcr = &mds->dc_region[i];
> > +
> > +		rc = add_dpa_res(dev, &cxlds->dpa_res, &cxlds->dc_res[i],
> > +				 dcr->base, dcr->decode_len, dcr->name);
> > +		if (rc)
> > +			return rc;
> > +	}
> >   
> >   	if (mds->partition_align_bytes == 0) {
> >   		rc = add_dpa_res(dev, &cxlds->dpa_res, &cxlds->ram_res, 0,
> >   				 mds->volatile_only_bytes, "ram");
> >   		if (rc)
> >   			return rc;
> > +
> 
> Stray blank line?

Ah yep!  Fixed!

Thanks for looking!
Ira