Re: [INTERNAL PATCH 2/2] cxl/cxlmem: Change cxl_mem to a more descriptive name

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Tue, 2 Nov 2021 13:29:01 -0700
<ira.weiny@intel.com> wrote:

> From: Ira Weiny <ira.weiny@intel.com>
> 
> The cxl_mem object actually represents the state of a CXL device within the
> driver.  This is data layered on top of the PCI or other device.  Even though
> the only device currently represented is a memory device, calling this cxl_mem
> is confusing because this is not solely a CXL memory device.  Furthermore,
> cxl_memdev does drive a CXL memory device.  So changing the name helps to
> distinguish these to structures.
> 
> Update the structure name, function names, and the kdocs to reflect the
> real uses of this structure.
> 
> Acked-by: Ben Widawsky <ben.widawsky@intel.com>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> 
...
Hi Ira,

>  /**
>   * struct cxl_mbox_cmd - A command to be submitted to hardware.
> @@ -90,8 +90,13 @@ struct cxl_mbox_cmd {
>  #define CXL_CAPACITY_MULTIPLIER SZ_256M
>  
>  /**
> - * struct cxl_mem - A CXL memory device
> - * @dev: The device associated with this CXL device.
> + * struct cxl_dev_state - The driver device state
> + *
> + * cxl_dev_state represents the CXL driver/device state.  It provides an
> + * interface to mailbox commands as well as some cached data about the device.
> + * Currently only memory devices are represented.
> + *
> + * @dev: The device associated with this CXL state
>   * @cxlmd: Logical memory device chardev / interface
>   * @regs: Parsed register blocks
>   * @payload_size: Size of space for payload
> @@ -117,7 +122,7 @@ struct cxl_mbox_cmd {
>   * See section 8.2.9.5.2 Capacity Configuration and Label Storage for
>   * details on capacity parameters.
>   */
> -struct cxl_mem {
> +struct cxl_dev_state {
>  	struct device *dev;
>  	struct cxl_memdev *cxlmd;

This patch got me thinking about whether the new name should have some
reference to it being the dev state for a type 3 device, because of this
pointer the cxlmd making me think this wasn't generic enough to be called
just cxl_dev_state.

Naturally next step was to wonder what this was used for....

Far as I can tell it isn't set or used.  Can we drop this pointer?
That would make it more obvious this simply provides services to the cxl_memdev
as there isn't any path for things to flow the other way.

At that point, there are still some memory device specific items in here
but they are all caches of things we have read from the device so they
don't bother me that much.

Jonathan

>  
> @@ -142,7 +147,7 @@ struct cxl_mem {
>  	u64 next_volatile_bytes;
>  	u64 next_persistent_bytes;
>  
> -	int (*mbox_send)(struct cxl_mem *cxlm, struct cxl_mbox_cmd *cmd);
> +	int (*mbox_send)(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *cmd);
>  };
...