Hi Rex,
On Mon, 2023-12-04 at 23:00 +0800, Rex Zhang wrote:
> 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()?
Yeah, I suppose it makes more sense to have it below anyway, will move
it there.
> > + 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()?
Same.
> > +
> > + 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.
No, this is required by review input from a previous version - the
compression device can only be unregistered on module exit.
Thanks,
Tom
> > 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
> >
