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
>
