Re: [PATCH v2 04/10] mpi3mr: Fix NVMe page size caching for non-operational devices

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The `reset_to` and `abort_to` fields are left uninitialized for PCIe devices in error states, causing task management commands to time out instantly and trigger unintended full controller resets.
--

--- Patch [4]: [PATCH v2 04/10] mpi3mr: Fix NVMe page size caching for non-operational devices ---
commit a648ff0d4d5627aa485822e3033940c480028902
Author: Ranjan Kumar <ranjan.kumar@broadcom.com>

mpi3mr: Fix NVMe page size caching for non-operational devices

This patch populates the PCIe page size attribute for NVMe devices regardless
of their access status, ensuring management IOCTLs can reach the firmware.
It also adds bounds checking for the firmware-provided page size to prevent
undefined shift behavior, falling back to a default size if invalid.

Closes: https://sashiko.dev/#/patchset/20260626114109.43685-1-ranjan.kumar@broadcom.com?part=4

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
> index d2a20f2721dbf..df7365d19b44a 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_os.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
> @@ -1354,12 +1354,14 @@ static void mpi3mr_update_tgtdev(struct mpi3mr_ioc *mrioc,
>  		tgtdev->dev_spec.pcie_inf.capb =
>  		    le32_to_cpu(pcieinf->capabilities);
>  		tgtdev->dev_spec.pcie_inf.mdts = MPI3MR_DEFAULT_MDTS;
> -		/* 2^12 = 4096 */
> -		tgtdev->dev_spec.pcie_inf.pgsz = 12;
> +		/* Validate firmware page size to prevent undefined shift behavior */
> +		if (pcieinf->page_size > 0 && pcieinf->page_size < 31)
> +			tgtdev->dev_spec.pcie_inf.pgsz = pcieinf->page_size;
> +		else
> +			tgtdev->dev_spec.pcie_inf.pgsz = 12; /* Default to 4096 (2^12) */
>  		if (dev_pg0->access_status == MPI3_DEVICE0_ASTATUS_NO_ERRORS) {
>  			tgtdev->dev_spec.pcie_inf.mdts =
>  			    le32_to_cpu(pcieinf->maximum_data_transfer_size);
> -			tgtdev->dev_spec.pcie_inf.pgsz = pcieinf->page_size;
>  			tgtdev->dev_spec.pcie_inf.reset_to =
>  			    max_t(u8, pcieinf->controller_reset_to,
>  			     MPI3MR_INTADMCMD_TIMEOUT);

[Severity: High]
This is a pre-existing issue, but does exposing the device to IOCTLs by
setting the page size here unintentionally turn a device error into a full
controller reset?

When an NVMe device is in an error state (e.g., NEEDS_INITIALIZATION),
access_status != NO_ERRORS, so reset_to and abort_to are left uninitialized (0).

With this patch populating pgsz for these devices, management IOCTLs can pass
the driver's PRP validation and reach the firmware. If the IOCTL times out
because the device is in an error state, the driver attempts recovery:

drivers/scsi/mpi3mr/mpi3mr_os.c:mpi3mr_issue_tm() {
    ...
	if (tgtdev) {
		if (tgtdev->dev_type == MPI3_DEVICE_DEVFORM_PCIE)
			timeout = cmd_priv ? tgtdev->dev_spec.pcie_inf.abort_to
					   : tgtdev->dev_spec.pcie_inf.reset_to;
    ...
}

Since reset_to and abort_to are 0, wait_for_completion_timeout() fails
instantly, causing the driver to falsely detect a TM timeout and
unconditionally execute mpi3mr_soft_reset_handler(), resetting the entire
controller.

Should reset_to and abort_to also be initialized with defaults even when the
device is in an error state to prevent routine monitoring tools from triggering
an HBA reset?

>  			tgtdev->dev_spec.pcie_inf.abort_to =
>  			    max_t(u8, pcieinf->nvme_abort_to,
>  			    MPI3MR_INTADMCMD_TIMEOUT);
>  		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708183305.244485-1-ranjan.kumar@broadcom.com?part=4