On Tue, 7 Nov 2023 at 10:13, Michael S. Tsirkin <m...@redhat.com> wrote:
>
> From: Davidlohr Bueso <d...@stgolabs.net>
>
> Make use of the background operations through the sanitize command, per CXL
> 3.0 specs. Traditionally run times can be rather long, depending on the
> size of the media.
>
> Estimate times based on:
> https://pmem.io/documents/NVDIMM_DSM_Interface-V1.8.pdf
>
Hi; Coverity points out dead code in this function (CID 1523905):
> +/*
> + * CXL 3.0 spec section 8.2.9.8.5.1 - Sanitize.
> + *
> + * Once the Sanitize command has started successfully, the device shall be
> + * placed in the media disabled state. If the command fails or is interrupted
> + * by a reset or power failure, it shall remain in the media disabled state
> + * until a successful Sanitize command has been completed. During this state:
> + *
> + * 1. Memory writes to the device will have no effect, and all memory reads
> + * will return random values (no user data returned, even for locations that
> + * the failed Sanitize operation didn’t sanitize yet).
> + *
> + * 2. Mailbox commands shall still be processed in the disabled state, except
> + * that commands that access Sanitized areas shall fail with the Media
> Disabled
> + * error code.
> + */
> +static CXLRetCode cmd_sanitize_overwrite(const struct cxl_cmd *cmd,
> + uint8_t *payload_in,
> + size_t len_in,
> + uint8_t *payload_out,
> + size_t *len_out,
> + CXLCCI *cci)
> +{
> + CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> + uint64_t total_mem; /* in Mb */
> + int secs;
> +
> + total_mem = (ct3d->cxl_dstate.vmem_size + ct3d->cxl_dstate.pmem_size) >>
> 20;
> + if (total_mem <= 512) {
> + secs = 4;
> + } else if (total_mem <= 1024) {
> + secs = 8;
> + } else if (total_mem <= 2 * 1024) {
> + secs = 15;
> + } else if (total_mem <= 4 * 1024) {
> + secs = 30;
> + } else if (total_mem <= 8 * 1024) {
> + secs = 60;
> + } else if (total_mem <= 16 * 1024) {
> + secs = 2 * 60;
> + } else if (total_mem <= 32 * 1024) {
> + secs = 4 * 60;
> + } else if (total_mem <= 64 * 1024) {
> + secs = 8 * 60;
> + } else if (total_mem <= 128 * 1024) {
> + secs = 15 * 60;
> + } else if (total_mem <= 256 * 1024) {
> + secs = 30 * 60;
> + } else if (total_mem <= 512 * 1024) {
> + secs = 60 * 60;
> + } else if (total_mem <= 1024 * 1024) {
> + secs = 120 * 60;
> + } else {
> + secs = 240 * 60; /* max 4 hrs */
> + }
Here we have an exhaustive if ladder that sets 'secs'. None
of the values we might end up with are less than 4.
> +
> + /* EBUSY other bg cmds as of now */
> + cci->bg.runtime = secs * 1000UL;
> + *len_out = 0;
> +
> + cxl_dev_disable_media(&ct3d->cxl_dstate);
> +
> + if (secs > 2) {
> + /* sanitize when done */
> + return CXL_MBOX_BG_STARTED;
> + } else {
...but here we have an else clause for when secs <= 2,
which can never happen.
> + __do_sanitization(ct3d);
> + cxl_dev_enable_media(&ct3d->cxl_dstate);
> +
> + return CXL_MBOX_SUCCESS;
> + }
What was the intention here ?
> +}
thanks
-- PMM
