Re: [PATCH v2] mpt3sas: Limit NVMe request size to 2 MiB

[Damien Le Moal <dlemoal@kernel.org>]

On 4/11/26 10:00, Ranjan Kumar wrote:
> Some firmware reports NVMe maximum transfer sizes that follow the drive
> capability. When those values are very large, the block layer may build
> I/O that this driver cannot handle, which can cause a kernel oops.
> 
> When an NVMe device is set up, cap how large a single transfer may be
> to the smaller of the firmware-reported limit and roughly two mebibytes
> with a small margin. If no valid limit is reported, apply the same
> upper bound.

What margin ? I do not see any...

> 
> Cc: stable@vger.kernel.org
> Fixes: 9b8b84879d4a ("block: Increase BLK_DEF_MAX_SECTORS_CAP")
> Reported-by: Mira Limbeck <m.limbeck@proxmox.com>
> Closes: https://lore.kernel.org/r/291f78bf-4b4a-40dd-867d-053b36c564b3@proxmox.com
> Link: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=9b8b84879d4a
> Suggested-by: Keith Busch <kbusch@kernel.org>
> Signed-off-by: Ranjan Kumar <ranjan.kumar@broadcom.com>
> ---
>  drivers/scsi/mpt3sas/mpt3sas_scsih.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> index 6ff788557294..fca9d6722fc8 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> @@ -54,6 +54,7 @@
>  #include <linux/interrupt.h>
>  #include <linux/raid_class.h>
>  #include <linux/unaligned.h>
> +#include <linux/sizes.h>
>  
>  #include "mpt3sas_base.h"
>  
> @@ -2737,9 +2738,17 @@ scsih_sdev_configure(struct scsi_device *sdev, struct queue_limits *lim)
>  				"connector name( %s)\n", ds,
>  				pcie_device->enclosure_level,
>  				pcie_device->connector_name);
> -

Spurious whiteline change. The white line is nice before the big block below.

> +		/*
> +		 * Firmware may report large NVMe MDTS values on some ASICs.

What ASICs ? The SSD controller or the HBA controller ? Also, does the HBA
firmware change the MDTS ? Or does it report the SSD reported MDTS as is ? If it
is the former, then an explanation would be nice. If it is the latter, instead
of "Firmware may report" I suggest "The NVMe device controller may report"

> +		 * Limit max_hw_sectors to the smaller of the reported MDTS
> +		 * and 2 MiB to avoid issuing I/O the driver cannot handle.

Without any explanations, 2MiB appears to be a "magic" value here. There is a
clear explanation for it with the 4K device page size that can fit 512 PRP
entries each pointing to one 4K page. So let's state that.

> +		 */
>  		if (pcie_device->nvme_mdts)
> -			lim->max_hw_sectors = pcie_device->nvme_mdts / 512;
> +			lim->max_hw_sectors = min_t(u32,
> +					pcie_device->nvme_mdts / 512,
> +					(SZ_2M / 512));
> +		else
> +			lim->max_hw_sectors = (SZ_2M / 512);

		lim->max_hw_sectors = SZ_2M >> SECTOR_SHIFT;
		if (pcie_device->nvme_mdts)
			lim->max_hw_sectors = min_t(u32, lim->max_hw_sectors,
					pcie_device->nvme_mdts >> SECTOR_SHIFT);

is I think a bit nicer.		

>  
>  		pcie_device_put(pcie_device);
>  		spin_unlock_irqrestore(&ioc->pcie_device_lock, flags);


-- 
Damien Le Moal
Western Digital Research