Re: [PATCH 2/4] cxl/mbox: Enable cxl_mbox_send_cmd() users to validate output size

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

On Mon, 05 Dec 2022 20:22:33 -0800
Dan Williams <dan.j.williams@intel.com> wrote:

> Internally cxl_mbox_send_cmd() converts all passed-in parameters to a
> 'struct cxl_mbox_cmd' instance and sends that to cxlds->mbox_send(). It
> then teases the possibilty that the caller can validate the output size.
> However, they cannot since the resulting output size is not conveyed to
> the called. Fix that by making the caller pass in a constructed 'struct
> cxl_mbox_cmd'. This prepares for a future patch to add output size
> validation on a per-command basis.
> 
> Given the change in signature, also change the name to differentiate it
> from the user command submission path that performs more validation
> before generating the 'struct cxl_mbox_cmd' instance to execute.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

I'm with Ira on ordering in structure alignment. Other trivial things inline.

Usual moans about slipping in unrelated tidy up.  All good stuff, but
would have preferred them separated out.  In interests of speed though
I'm fine with it as it stands

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

>  static int __cxl_pmem_security_disable(struct nvdimm *nvdimm,
> @@ -83,15 +95,21 @@ static int __cxl_pmem_security_disable(struct nvdimm *nvdimm,
>  	struct cxl_memdev *cxlmd = cxl_nvd->cxlmd;
>  	struct cxl_dev_state *cxlds = cxlmd->cxlds;
>  	struct cxl_disable_pass dis_pass;
> -	int rc;
> +	struct cxl_mbox_cmd mbox_cmd;
>  
> -	dis_pass.type = ptype == NVDIMM_MASTER ?
> -		CXL_PMEM_SEC_PASS_MASTER : CXL_PMEM_SEC_PASS_USER;
> +	dis_pass = (struct cxl_disable_pass) {
> +		.type = ptype == NVDIMM_MASTER ? CXL_PMEM_SEC_PASS_MASTER :
> +						 CXL_PMEM_SEC_PASS_USER,
> +	};

Unrelated change as far as I can see. Perhaps reasonable to keep a common
approach, but should really be in a precusor patch.


> @@ -140,14 +167,22 @@ static int cxl_pmem_security_passphrase_erase(struct nvdimm *nvdimm,
>  	struct cxl_nvdimm *cxl_nvd = nvdimm_provider_data(nvdimm);
>  	struct cxl_memdev *cxlmd = cxl_nvd->cxlmd;
>  	struct cxl_dev_state *cxlds = cxlmd->cxlds;
> +	struct cxl_mbox_cmd mbox_cmd;
>  	struct cxl_pass_erase erase;
>  	int rc;
>  
> -	erase.type = ptype == NVDIMM_MASTER ?
> -		CXL_PMEM_SEC_PASS_MASTER : CXL_PMEM_SEC_PASS_USER;
> +	erase = (struct cxl_pass_erase) {
> +		.type = ptype == NVDIMM_MASTER ? CXL_PMEM_SEC_PASS_MASTER :
> +						 CXL_PMEM_SEC_PASS_USER,
> +	};

Unrelated change?  Meh trivial though so fair enough rolling it in here.


>  	memcpy(erase.pass, key->data, NVDIMM_PASSPHRASE_LEN);
> -	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_PASSPHRASE_SECURE_ERASE,
> -			       &erase, sizeof(erase), NULL, 0);
> +	mbox_cmd = (struct cxl_mbox_cmd) {
> +		.opcode = CXL_MBOX_OP_PASSPHRASE_SECURE_ERASE,
> +		.size_in = sizeof(erase),
> +		.payload_in = &erase,
> +	};
> +
> +	rc = cxl_internal_send_cmd(cxlds, &mbox_cmd);
>  	if (rc < 0)
>  		return rc;
>  
>