Re: [PATCH RFC 02/15] tools/testing/cxl: Create context for cxl mock device

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

On Fri, 15 Jul 2022 14:08:44 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> Add context struct for mock device and move lsa under the context. This
> allows additional information such as security status and other persistent
> security data such as passphrase to be added for the emulated test device.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  tools/testing/cxl/test/mem.c |   29 +++++++++++++++++++++++------
>  1 file changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c
> index 6b9239b2afd4..723378248321 100644
> --- a/tools/testing/cxl/test/mem.c
> +++ b/tools/testing/cxl/test/mem.c
> @@ -9,6 +9,10 @@
>  #include <linux/bits.h>
>  #include <cxlmem.h>
>  
> +struct mock_mdev_data {
> +	void *lsa;
> +};
> +
>  #define LSA_SIZE SZ_128K
>  #define EFFECT(x) (1U << x)
>  
> @@ -140,7 +144,8 @@ static int mock_id(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *cmd)
>  static int mock_get_lsa(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *cmd)
>  {
>  	struct cxl_mbox_get_lsa *get_lsa = cmd->payload_in;
> -	void *lsa = dev_get_drvdata(cxlds->dev);
> +	struct mock_mdev_data *mdata = dev_get_drvdata(cxlds->dev);
> +	void *lsa = mdata->lsa;
>  	u32 offset, length;
>  
>  	if (sizeof(*get_lsa) > cmd->size_in)
> @@ -159,7 +164,8 @@ static int mock_get_lsa(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *cmd)
>  static int mock_set_lsa(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *cmd)
>  {
>  	struct cxl_mbox_set_lsa *set_lsa = cmd->payload_in;
> -	void *lsa = dev_get_drvdata(cxlds->dev);
> +	struct mock_mdev_data *mdata = dev_get_drvdata(cxlds->dev);
> +	void *lsa = mdata->lsa;
>  	u32 offset, length;
>  
>  	if (sizeof(*set_lsa) > cmd->size_in)
> @@ -237,9 +243,12 @@ static int cxl_mock_mbox_send(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *
>  	return rc;
>  }
>  
> -static void label_area_release(void *lsa)
> +static void cxl_mock_drvdata_release(void *data)
>  {
> -	vfree(lsa);
> +	struct mock_mdev_data *mdata = data;
> +
> +	vfree(mdata->lsa);
> +	vfree(mdata);
>  }
>  
>  static int cxl_mock_mem_probe(struct platform_device *pdev)
> @@ -247,13 +256,21 @@ static int cxl_mock_mem_probe(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	struct cxl_memdev *cxlmd;
>  	struct cxl_dev_state *cxlds;
> +	struct mock_mdev_data *mdata;
>  	void *lsa;
>  	int rc;
>  
> +	mdata = vmalloc(sizeof(*mdata));

It's tiny so why vmalloc?  I guess that might become apparent later.
devm_kzalloc() should be fine and lead to simpler error handling.

> +	if (!mdata)
> +		return -ENOMEM;
> +
>  	lsa = vmalloc(LSA_SIZE);
> -	if (!lsa)
> +	if (!lsa) {
> +		vfree(mdata);
In general doing this just makes things fragile in the long term. Better to
register one devm_add_action_or_reset() for each thing set up (or standard
allcoation).

>  		return -ENOMEM;
> -	rc = devm_add_action_or_reset(dev, label_area_release, lsa);
> +	}
> +
> +	rc = devm_add_action_or_reset(dev, cxl_mock_drvdata_release, mdata);
>  	if (rc)
>  		return rc;
>  	dev_set_drvdata(dev, lsa);
> 
>