RE: [PATCH 3/7] irqbypass: Take ownership of producer/consumer token tracking

[Tian, Kevin]

> From: Sean Christopherson <seanjc@xxxxxxxxxx>
> Sent: Saturday, April 5, 2025 5:15 AM
> 
> @@ -505,15 +505,12 @@ static int vfio_msi_set_vector_signal(struct
> vfio_pci_core_device *vdev,
>  	if (ret)
>  		goto out_put_eventfd_ctx;
> 
> -	ctx->producer.token = trigger;
>  	ctx->producer.irq = irq;
> -	ret = irq_bypass_register_producer(&ctx->producer);
> +	ret = irq_bypass_register_producer(&ctx->producer, trigger);
>  	if (unlikely(ret)) {
>  		dev_info(&pdev->dev,
>  		"irq bypass producer (token %p) registration fails: %d\n",
>  		ctx->producer.token, ret);

Use 'trigger' as ctx->producer.token is NULL if registration fails. 

> @@ -18,20 +19,20 @@ struct irq_bypass_consumer;
>   * The IRQ bypass manager is a simple set of lists and callbacks that allows
>   * IRQ producers (ex. physical interrupt sources) to be matched to IRQ
>   * consumers (ex. virtualization hardware that allows IRQ bypass or offload)
> - * via a shared token (ex. eventfd_ctx).  Producers and consumers register
> - * independently.  When a token match is found, the optional @stop
> callback
> - * will be called for each participant.  The pair will then be connected via
> - * the @add_* callbacks, and finally the optional @start callback will allow
> - * any final coordination.  When either participant is unregistered, the
> - * process is repeated using the @del_* callbacks in place of the @add_*
> - * callbacks.  Match tokens must be unique per producer/consumer, 1:N
> pairings
> - * are not supported.
> + * via a shared eventfd_ctx).  Producers and consumers register
> independently.

s/eventfd_ctx)/eventfd_ctx/

> +int irq_bypass_register_consumer(struct irq_bypass_consumer *consumer,
> +				 struct eventfd_ctx *eventfd)
>  {
>  	struct irq_bypass_consumer *tmp;
>  	struct irq_bypass_producer *producer;
>  	int ret;
> 
> -	if (!consumer->token ||
> -	    !consumer->add_producer || !consumer->del_producer)
> +	if (WARN_ON_ONCE(consumer->token))
> +		return -EINVAL;
> +
> +	if (!consumer->add_producer || !consumer->del_producer)
>  		return -EINVAL;
> 
>  	mutex_lock(&lock);
> 
>  	list_for_each_entry(tmp, &consumers, node) {
> -		if (tmp->token == consumer->token || tmp == consumer) {
> +		if (tmp->token == eventfd || tmp == consumer) {
>  			ret = -EBUSY;
>  			goto out_err;
>  		}
>  	}

the 2nd check 'tmp == consumer' is redundant. If they are equal 
consumer->token is not NULL then the earlier WARN_ON will be
triggered already.

otherwise,

Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx>