Re: [PATCH V3] cxl/mem: Account for partitionable space in ram/pmem ranges

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

On Fri, Jun 18, 2021 at 09:58:33AM -0700, Widawsky, Ben wrote:
> On 21-06-18 09:31:29, ira.weiny@intel.com wrote:
> > From: Ira Weiny <ira.weiny@intel.com>
> > 
> > Memory devices may specify volatile only, persistent only, and
> > partitionable space which when added together result in a total capacity.
> > 
> > If Identify Memory Device.Partition Alignment != 0 the device supports
> > partitionable space.  This partitionable space can be split between
> > volatile and persistent space.  The total volatile and persistent sizes
> > are reported in Get Partition Info.  ie
> > 
> > 	active volatile memory = volatile only + partitionable volatile
> > 	active persistent memory = persistent only + partitionable persistent
> > 
> > Define cxl_mem_get_partition(), check for partitionable support, and use
> > cxl_mem_get_partition() if applicable.
> 
> This doesn't look right to me, but I'll happily stand corrected.
> 

[snip]

> > +
> > +static int cxl_mem_create_range_info(struct cxl_mem *cxlm)
> > +{
> > +	int rc;
> > +
> > +	if (cxlm->partition_align_bytes == 0) {
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This...

> > +		cxlm->ram_range.start = 0;
> > +		cxlm->ram_range.end = cxlm->volatile_only_bytes - 1;
> > +		cxlm->pmem_range.start = 0;
> > +		cxlm->pmem_range.end = cxlm->persistent_only_bytes - 1;
> > +		return 0;
> > +	}
> > +
> > +	rc = cxl_mem_get_partition_info(cxlm,
> > +					&cxlm->active_volatile_bytes,
> > +					&cxlm->active_persistent_bytes,
> > +					&cxlm->next_volatile_bytes,
> > +					&cxlm->next_persistent_bytes);
> > +	if (rc < 0) {
> > +		dev_err(&cxlm->pdev->dev, "Failed to query partition information\n");
> > +		return rc;
> > +	}
> 
> If the command is not supported, you're going to get back -ENXIO here and fail.
> I think what you want to do also is use IDENTIFY if PARTITION_INFO doesn't
> exist.

... should handle the situation where Get Partition Info is not supported.

I did not see it explicitly stated but I believe if Partition Align Bytes is
non-0 then partitioning is supported and Get/Set Partition Info should work.

> 
> Assuming my assessment is correct, I think it's time to make
> cxl_mem_mbox_send_cmd grow a little bit better error handling. I'd recommend
> something like this and then checking for -ENOTSUPP.
> 
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index b1a5a18dba92..3b7d8a905393 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -92,6 +92,7 @@ struct mbox_cmd {
>         size_t size_out;
>         u16 return_code;
>  #define CXL_MBOX_SUCCESS 0
> +#define CXL_MBOX_UNSUPPORTED 0x15
>  };
> 
>  static int cxl_mem_major;
> @@ -876,9 +877,15 @@ static int cxl_mem_mbox_send_cmd(struct cxl_mem *cxlm, u16 opcode,
>         if (rc)
>                 return rc;
> 
> -       /* TODO: Map return code to proper kernel style errno */
> -       if (mbox_cmd.return_code != CXL_MBOX_SUCCESS)
> +       switch (mbox_cmd.return_code) {
> +       case CXL_MBOX_SUCCESS:
> +               break;
> +       case CXL_MBOX_UNSUPPORTED:
> +               return -ENOTSUPP;
> +       default:
> +               /* TODO: Map return code to proper kernel style errno */
>                 return -ENXIO;
> +       }


I'm not opposed to adding this though.

Ira