Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
From:   Rex Zhang <rex.zhang@intel.com>
To:     tom.zanussi@linux.intel.com
Cc:     dave.jiang@intel.com, davem@davemloft.net,
        dmaengine@vger.kernel.org, fenghua.yu@intel.com,
        giovanni.cabiddu@intel.com, herbert@gondor.apana.org.au,
        james.guilford@intel.com, kanchana.p.sridhar@intel.com,
        linux-crypto@vger.kernel.org, linux-kernel@vger.kernel.org,
        pavel@ucw.cz, tony.luck@intel.com, vinodh.gopal@intel.com,
        vkoul@kernel.org, wajdi.k.feghali@intel.com
Subject: Re: [PATCH v11 11/14] crypto: iaa - Add support for deflate-iaa compression algorithm
Date:   Mon,  4 Dec 2023 23:00:28 +0800
Message-Id: <20231204150028.3544490-1-rex.zhang@intel.com>
In-Reply-To: <20231201201035.172465-12-tom.zanussi@linux.intel.com>
References: <20231201201035.172465-12-tom.zanussi@linux.intel.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi, Tom,

On 2023-12-01 at 14:10:32 -0600, Tom Zanussi wrote:

[snip]

> +static int iaa_wq_put(struct idxd_wq *wq)
> +{
> +	struct idxd_device *idxd = wq->idxd;
> +	struct iaa_wq *iaa_wq;
> +	bool free = false;
> +	int ret = 0;
> +
> +	spin_lock(&idxd->dev_lock);
> +	iaa_wq = idxd_wq_get_private(wq);
> +	if (iaa_wq) {
> +		iaa_wq->ref--;
> +		if (iaa_wq->ref == 0 && iaa_wq->remove) {
> +			__free_iaa_wq(iaa_wq);
> +			idxd_wq_set_private(wq, NULL);
> +			free = true;
> +		}
> +		idxd_wq_put(wq);
> +	} else {
> +		ret = -ENODEV;
> +	}
> +	spin_unlock(&idxd->dev_lock);
__free_iaa_wq() may cause schedule, whether it should be move out of the
context between spin_lock() and spin_unlock()?
> +	if (free)
> +		kfree(iaa_wq);
> +
> +	return ret;
> +}

[snip]

> @@ -800,12 +1762,38 @@ static void iaa_crypto_remove(struct idxd_dev *idxd_dev)
>  
>  	remove_iaa_wq(wq);
>  
> +	spin_lock(&idxd->dev_lock);
> +	iaa_wq = idxd_wq_get_private(wq);
> +	if (!iaa_wq) {
> +		spin_unlock(&idxd->dev_lock);
> +		pr_err("%s: no iaa_wq available to remove\n", __func__);
> +		goto out;
> +	}
> +
> +	if (iaa_wq->ref) {
> +		iaa_wq->remove = true;
> +	} else {
> +		wq = iaa_wq->wq;
> +		__free_iaa_wq(iaa_wq);
> +		idxd_wq_set_private(wq, NULL);
> +		free = true;
> +	}
> +	spin_unlock(&idxd->dev_lock);
__free_iaa_wq() may cause schedule, whether it should be move out of the
context between spin_lock() and spin_unlock()?
> +
> +	if (free)
> +		kfree(iaa_wq);
> +
>  	idxd_drv_disable_wq(wq);
>  	rebalance_wq_table();
>  
> -	if (nr_iaa == 0)
> +	if (nr_iaa == 0) {
> +		iaa_crypto_enabled = false;
Is it necessary to add iaa_unregister_compression_device() here?
All iaa devices are disabled cause the variable first_wq will be true,
if enable wq, iaa_register_compression_device() will fail due to the
algorithm is existed.
>  		free_wq_table();
> +		module_put(THIS_MODULE);
>  
> +		pr_info("iaa_crypto now DISABLED\n");
> +	}
> +out:
>  	mutex_unlock(&iaa_devices_lock);
>  	mutex_unlock(&wq->wq_lock);
>  }

[snip]

Thanks,
Rex Zhang
> -- 
> 2.34.1
>