Re: [PATCH v10 6/9] scsi: ibmvfc: extend channel reg/dereg helpers for async sub-CRQ

[Tyrel Datwyler]

On 9/10/26 10:48 PM, Tyrel Datwyler wrote:
> From: Dave Marquardt <davemarq@xxxxxxxxxxxxx>
> 
> ibmvfc_register_channel() and ibmvfc_deregister_channel() previously only
> handled indexed sub-CRQ channels drawn from the channels->scrqs[] array.
> The async sub-CRQ (vhost->async_sub_crq) had no registration path through
> these helpers, requiring separate handling.
> 
> Extend both functions to accept a negative index as a sentinel value
> signalling that the async sub-CRQ should be operated on instead of an
> indexed scrq entry. When index < 0, the queue pointer is set to
> &vhost->async_sub_crq, the IRQ is named "ibmvfc-<addr>-async", and the
> handler is set to ibmvfc_interrupt_async_subq rather than the per-protocol
> ibmvfc_interrupt_mq handler. hwq_id assignment is skipped for the async
> queue since it has no meaningful hardware queue index.
> 
> Stopped marking ibmvfc_interrupt_async_subq as __maybe_unused.
> 
> Error messages in both paths are updated to distinguish async sub-CRQ
> failures from indexed sub-CRQ failures. Kernel-doc headers are added to
> both functions documenting the negative-index convention.
> 
> Signed-off-by: Dave Marquardt <davemarq@xxxxxxxxxxxxx>
> Acked-by: Tyrel Datwyler <tyreld@xxxxxxxxxxxxx>

I was a little relectuant about the chages here initially, and the more I look
at this the more I don't like it. So I think I'm changing my mind to a NACK here.

We want channel registration to be generic and the using a negative index to
identify an async subq seems like a hack. The reality is there is only one
channel and its at index 0. Futher, the async events are there own protocol that
we process in the client so treat them as such and use the channel protocol
field to identify that this is an async subq.

-Tyrel
> ---
>  drivers/scsi/ibmvscsi/ibmvfc-core.c | 93 ++++++++++++++++++++++-------
>  1 file changed, 70 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> index 670f6b3a5476..a9cf1096e755 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> @@ -4419,7 +4419,7 @@ static void ibmvfc_drain_async_subq(struct ibmvfc_queue *scrq)
>   * @scrq_instance: async subq
>   *
>   **/
> -static irqreturn_t __maybe_unused ibmvfc_interrupt_async_subq(int irq, void *scrq_instance)
> +static irqreturn_t ibmvfc_interrupt_async_subq(int irq, void *scrq_instance)
>  {
>  	struct ibmvfc_queue *scrq = (struct ibmvfc_queue *)scrq_instance;
>  
> @@ -6810,13 +6810,29 @@ static int ibmvfc_init_crq(struct ibmvfc_host *vhost)
>  	return retrc;
>  }
>  
> +/**
> + * ibmvfc_register_channel - Register a sub-CRQ channel with the hypervisor
> + * @vhost:	ibmvfc host struct
> + * @channels:	ibmvfc channels struct containing the channel array and protocol
> + * @index:	index into the channels array for the queue to register, or
> + *		a negative value to register the async sub-CRQ
> + *
> + * Register a sub-CRQ with the hypervisor via h_reg_sub_crq, map its hardware
> + * IRQ to a Linux IRQ, and bind an interrupt handler to it. The handler is
> + * selected based on the channel protocol (SCSI or NVMe) for normal queues, or
> + * set to the async sub-CRQ handler when @index is negative.
> + *
> + * Return value:
> + *	0 on success / non-zero on failure
> + **/
>  static int ibmvfc_register_channel(struct ibmvfc_host *vhost,
>  				   struct ibmvfc_channels *channels,
>  				   int index)
>  {
>  	struct device *dev = vhost->dev;
>  	struct vio_dev *vdev = to_vio_dev(dev);
> -	struct ibmvfc_queue *scrq = &channels->scrqs[index];
> +	bool is_async = index < 0;
> +	struct ibmvfc_queue *scrq = !is_async ? &channels->scrqs[index] : &vhost->async_sub_crq;
>  	int rc = -ENOMEM;
>  
>  	ENTER;
> @@ -6836,36 +6852,49 @@ static int ibmvfc_register_channel(struct ibmvfc_host *vhost,
>  
>  	if (!scrq->irq) {
>  		rc = -EINVAL;
> -		dev_err(dev, "Error mapping sub-crq[%d] irq\n", index);
> +		if (!is_async)
> +			dev_err(dev, "Error mapping sub-crq[%d] irq\n", index);
> +		else
> +			dev_err(dev, "Error mapping async sub-crq irq\n");
>  		goto irq_failed;
>  	}
>  
> -	switch (channels->protocol) {
> -	case IBMVFC_PROTO_SCSI:
> -		snprintf(scrq->name, sizeof(scrq->name), "ibmvfc-%x-scsi%d",
> -			 vdev->unit_address, index);
> -		scrq->handler = ibmvfc_interrupt_mq;
> -		break;
> -	case IBMVFC_PROTO_NVME:
> -		snprintf(scrq->name, sizeof(scrq->name), "ibmvfc-%x-nvmf%d",
> -			 vdev->unit_address, index);
> -		scrq->handler = ibmvfc_interrupt_mq;
> -		break;
> -	default:
> -		dev_err(dev, "Unknown channel protocol (%d)\n",
> -			channels->protocol);
> -		goto irq_failed;
> +	if (!is_async) {
> +		switch (channels->protocol) {
> +		case IBMVFC_PROTO_SCSI:
> +			snprintf(scrq->name, sizeof(scrq->name), "ibmvfc-%x-scsi%d",
> +				 vdev->unit_address, index);
> +			scrq->handler = ibmvfc_interrupt_mq;
> +			break;
> +		case IBMVFC_PROTO_NVME:
> +			snprintf(scrq->name, sizeof(scrq->name), "ibmvfc-%x-nvmf%d",
> +				 vdev->unit_address, index);
> +			scrq->handler = ibmvfc_interrupt_mq;
> +			break;
> +		default:
> +			dev_err(dev, "Unknown channel protocol (%d)\n",
> +				channels->protocol);
> +			goto irq_failed;
> +		}
> +	} else {
> +		snprintf(scrq->name, sizeof(scrq->name), "ibmvfc-%x-async",
> +			 vdev->unit_address);
> +		scrq->handler = ibmvfc_interrupt_async_subq;
>  	}
>  
>  	rc = request_irq(scrq->irq, scrq->handler, 0, scrq->name, scrq);
>  
>  	if (rc) {
> -		dev_err(dev, "Couldn't register sub-crq[%d] irq\n", index);
> +		if (!is_async)
> +			dev_err(dev, "Couldn't register sub-crq[%d] irq\n", index);
> +		else
> +			dev_err(dev, "Couldn't register async sub-crq irq\n");
>  		irq_dispose_mapping(scrq->irq);
>  		goto irq_failed;
>  	}
>  
> -	scrq->hwq_id = index;
> +	if (!is_async)
> +		scrq->hwq_id = index;
>  
>  	LEAVE;
>  	return 0;
> @@ -6879,13 +6908,26 @@ static int ibmvfc_register_channel(struct ibmvfc_host *vhost,
>  	return rc;
>  }
>  
> +/**
> + * ibmvfc_deregister_channel - Deregister a sub-CRQ channel with the hypervisor
> + * @vhost:	ibmvfc host struct
> + * @channels:	ibmvfc channels struct containing the sub-CRQ array
> + * @index:	index into the sub-CRQ array, or -1 to deregister the
> + *		asynchronous sub-CRQ
> + *
> + * Frees the IRQ, disposes of the IRQ mapping, and calls H_FREE_SUB_CRQ to
> + * release the sub-CRQ with the hypervisor. On success the queue message
> + * buffer is zeroed and the current index is reset. If H_FREE_SUB_CRQ fails,
> + * an error is logged but the channel resources are cleaned up regardless.
> + */
>  static void ibmvfc_deregister_channel(struct ibmvfc_host *vhost,
>  				      struct ibmvfc_channels *channels,
>  				      int index)
>  {
>  	struct device *dev = vhost->dev;
>  	struct vio_dev *vdev = to_vio_dev(dev);
> -	struct ibmvfc_queue *scrq = &channels->scrqs[index];
> +	bool is_async = index < 0;
> +	struct ibmvfc_queue *scrq = !is_async ? &channels->scrqs[index] : &vhost->async_sub_crq;
>  	long rc;
>  
>  	ENTER;
> @@ -6899,8 +6941,13 @@ static void ibmvfc_deregister_channel(struct ibmvfc_host *vhost,
>  					scrq->cookie);
>  	} while (rc == H_BUSY || H_IS_LONG_BUSY(rc));
>  
> -	if (rc)
> -		dev_err(dev, "Failed to free sub-crq[%d]: rc=%ld\n", index, rc);
> +	if (rc) {
> +		if (!is_async)
> +			dev_err(dev, "Failed to free sub-crq[%d]: rc=%ld\n",
> +				index, rc);
> +		else
> +			dev_err(dev, "Failed to free async sub-crq: rc=%ld\n", rc);
> +	}
>  
>  	/* Clean out the queue */
>  	memset(scrq->msgs.crq, 0, PAGE_SIZE);