Re: [PATCH 04/29] ibmvfc: initialize NVMe channel configuration during driver probe

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The user-configured module parameter for the number of NVMe hardware submission queues (`nvme_host_queues`) is ignored during NVMe queue initialization, preventing users from capping the maximum number of NVMe host queues.
- [Medium] Bitfield truncation of unsigned integer module parameters can silently disable NVMe or create an inconsistent state.
--

commit 8b0247aa812866f78c7701f707162de73d472772
Author: Tyrel Datwyler <tyreld@linux.ibm.com>

ibmvfc: initialize NVMe channel configuration during driver probe

Initialize the host's NVMe channel-group state during probe. This prepares the
driver with a NVMe/FC channel group that can will be used by later patches for
NVMe queue allocation, discovery buffers, target management, and IO
submission.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> @@ -6337,7 +6337,8 @@ static int ibmvfc_probe(struct vio_dev *vdev, const struct vio_device_id *id)
>  	struct device *dev = &vdev->dev;
>  	int rc = -ENOMEM;
>  	unsigned int online_cpus = num_online_cpus();
> -	unsigned int max_scsi_queues = min((unsigned int)IBMVFC_MAX_SCSI_QUEUES, online_cpus);
> +	unsigned int max_scsi_queues = min_t(unsigned int, IBMVFC_MAX_SCSI_QUEUES, online_cpus);
> +	unsigned int max_nvme_queues = min_t(unsigned int, IBMVFC_MAX_NVME_QUEUES, online_cpus);
>  
>  	ENTER;
>  	shost = scsi_host_alloc(&driver_template, sizeof(*vhost));

[ ... ]

> @@ -6371,6 +6373,10 @@ static int ibmvfc_probe(struct vio_dev *vdev, const struct vio_device_id *id)
>  	vhost->scsi_scrqs.protocol = IBMVFC_PROTO_SCSI;
>  	vhost->using_channels = 0;
>  	vhost->do_enquiry = 1;
> +	vhost->nvme_enabled = mq_enabled ? nvme_enabled : 0;

[Severity: Medium]
Could this result in unintended bitfield truncation if an even value is passed
via the module parameter?

Since vhost->nvme_enabled is a 1-bit bitfield, assigning an even value
directly will truncate it to 0, unintentionally disabling NVMe.

Additionally, since the ternary operator checks the un-normalized mq_enabled
integer directly, couldn't this create an inconsistent state where the ternary
evaluates to true, but the underlying vhost->mq_enabled field was truncated
to 0?

> +	vhost->nvme_scrqs.desired_queues = min(max_nvme_queues, nr_nvme_channels);
> +	vhost->nvme_scrqs.max_queues = max_nvme_queues;

[Severity: Medium]
Should the maximum queue calculations be constrained by the nr_nvme_hw_queues
module parameter?

The NVMe queue initialization currently ignores the user-configured
nr_nvme_hw_queues limit, unconditionally defaulting to the max_nvme_queues
limit based on IBMVFC_MAX_NVME_QUEUES and online CPUs.

> +	vhost->nvme_scrqs.protocol = IBMVFC_PROTO_NVME;
>  	vhost->scan_timeout = 0;
>  
>  	strcpy(vhost->partition_name, "UNKNOWN");

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623013035.3436640-1-tyreld@linux.ibm.com?part=4