Messages in this thread | | | Date | Thu, 15 Aug 2019 18:45:27 +0200 (CEST) | From | Thomas Gleixner <> | Subject | Re: [PATCH v3 3/3] vfio_pci: make use of update_irq_devid and optimize irq ops |
| |
On Thu, 15 Aug 2019, Ben Luo wrote: > if (vdev->ctx[vector].trigger) { > - free_irq(irq, vdev->ctx[vector].trigger); > - irq_bypass_unregister_producer(&vdev->ctx[vector].producer); > - kfree(vdev->ctx[vector].name); > - eventfd_ctx_put(vdev->ctx[vector].trigger); > - vdev->ctx[vector].trigger = NULL; > + if (vdev->ctx[vector].trigger == trigger) { > + irq_bypass_unregister_producer(&vdev->ctx[vector].producer);
What's the point of unregistering the producer, just to register it again below?
> + } else if (trigger) { > + ret = update_irq_devid(irq, > + vdev->ctx[vector].trigger, trigger); > + if (unlikely(ret)) { > + dev_info(&pdev->dev, > + "update_irq_devid %d (token %p) fails: %d\n",
s/fails/failed/
> + irq, vdev->ctx[vector].trigger, ret); > + eventfd_ctx_put(trigger); > + return ret; > + }
This lacks any form of comment why this is correct. dev_id is updated and the producer with the old token is still registered.
> + irq_bypass_unregister_producer(&vdev->ctx[vector].producer);
Now it's unregistered.
> + eventfd_ctx_put(vdev->ctx[vector].trigger); > + vdev->ctx[vector].producer.token = trigger;
The token is updated and then it's newly registered below.
> + vdev->ctx[vector].trigger = trigger; > - vdev->ctx[vector].producer.token = trigger; > - vdev->ctx[vector].producer.irq = irq; > + /* always update irte for posted mode */ > ret = irq_bypass_register_producer(&vdev->ctx[vector].producer); > if (unlikely(ret)) > dev_info(&pdev->dev, > "irq bypass producer (token %p) registration fails: %d\n", > vdev->ctx[vector].producer.token, ret);
I know this code already existed, but again this looks inconsistent. If the registration fails then a subsequent update will try to unregister a not registered producer. Does not make any sense to me.
Thanks,
tglx
| |