* [PATCH v4 01/10] mpi3mr: Skip device shutdown during unload per controller configuration
2026-08-05 11:06 [PATCH v4 00/10] mpi3mr: Few Enhancements and minor fixes Ranjan Kumar
@ 2026-08-05 11:06 ` Ranjan Kumar
2026-08-05 11:33 ` sashiko-bot
2026-08-05 11:06 ` [PATCH v4 02/10] mpi3mr: Update MPI Headers to revision 41 Ranjan Kumar
` (8 subsequent siblings)
9 siblings, 1 reply; 18+ messages in thread
From: Ranjan Kumar @ 2026-08-05 11:06 UTC (permalink / raw)
To: linux-scsi, martin.petersen
Cc: sathya.prakash, chandrakanth.patil, vishakhavc, ipylypiv,
Ranjan Kumar, Sashiko
The controller may be configured through Driver Page 1 to suppress
device shutdown requests during driver unload. Cache this setting and
skip the device shutdown request during IOC shutdown when unloading
the driver.
Additionally, ensure the driver_pg1 fields are properly converted
from little-endian to CPU endianness using le32_to_cpu() and le16_to_cpu()
before evaluating the shutdown disable flag and allocating diag buffers.
This prevents failures and massive memory allocation errors on big-endian
architectures.
Also harden the diagnostic buffer allocation retry loops against
invalid firmware-provided decrement sizes. The trace buffer loop
already guarded against a zero or oversized decrement size (infinite
loop or unsigned underflow). The firmware buffer loop had the same
gap and now carries the same guard.
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/patchset/20260626114109.43685-1-ranjan.kumar@broadcom.com?part=1
Closes: https://sashiko.dev/#/patchset/20260708183305.244485-1-ranjan.kumar@broadcom.com?part=1
Closes: https://sashiko.dev/#/patchset/20260724102505.115136-1-ranjan.kumar@broadcom.com?part=1
Signed-off-by: Chandrakanth Patil <chandrakanth.patil@broadcom.com>
Signed-off-by: Ranjan Kumar <ranjan.kumar@broadcom.com>
---
drivers/scsi/mpi3mr/mpi3mr.h | 3 +++
drivers/scsi/mpi3mr/mpi3mr_app.c | 44 ++++++++++++++++++++------------
drivers/scsi/mpi3mr/mpi3mr_fw.c | 35 ++++++++++++++++++-------
drivers/scsi/mpi3mr/mpi3mr_os.c | 2 ++
4 files changed, 58 insertions(+), 26 deletions(-)
diff --git a/drivers/scsi/mpi3mr/mpi3mr.h b/drivers/scsi/mpi3mr/mpi3mr.h
index c25525fe0671..39096004c60a 100644
--- a/drivers/scsi/mpi3mr/mpi3mr.h
+++ b/drivers/scsi/mpi3mr/mpi3mr.h
@@ -1410,6 +1410,9 @@ struct mpi3mr_ioc {
struct dma_pool *trace_buf_pool;
struct segments *trace_buf;
u8 invalid_io_comp;
+ bool is_unload;
+ bool skip_dev_shutdown_on_unload;
+
};
diff --git a/drivers/scsi/mpi3mr/mpi3mr_app.c b/drivers/scsi/mpi3mr/mpi3mr_app.c
index 1353a8ff9c85..fca5357a515d 100644
--- a/drivers/scsi/mpi3mr/mpi3mr_app.c
+++ b/drivers/scsi/mpi3mr/mpi3mr_app.c
@@ -141,25 +141,27 @@ void mpi3mr_alloc_diag_bufs(struct mpi3mr_ioc *mrioc)
trace_min_size = fw_min_size = MPI3MR_DEFAULT_HDB_MIN_SZ;
} else {
- trace_size = driver_pg1.host_diag_trace_max_size * 1024;
- trace_dec_size = driver_pg1.host_diag_trace_decrement_size
+ trace_size = le16_to_cpu(driver_pg1.host_diag_trace_max_size) * 1024;
+ trace_dec_size = le16_to_cpu(driver_pg1.host_diag_trace_decrement_size)
* 1024;
- trace_min_size = driver_pg1.host_diag_trace_min_size * 1024;
- fw_size = driver_pg1.host_diag_fw_max_size * 1024;
- fw_dec_size = driver_pg1.host_diag_fw_decrement_size * 1024;
- fw_min_size = driver_pg1.host_diag_fw_min_size * 1024;
+ trace_min_size = le16_to_cpu(driver_pg1.host_diag_trace_min_size) * 1024;
+ fw_size = le16_to_cpu(driver_pg1.host_diag_fw_max_size) * 1024;
+ fw_dec_size = le16_to_cpu(driver_pg1.host_diag_fw_decrement_size) * 1024;
+ fw_min_size = le16_to_cpu(driver_pg1.host_diag_fw_min_size) * 1024;
dprint_init(mrioc,
"%s:trace diag buffer sizes read from driver\n"
"page1: maximum size = %dKB, decrement size = %dKB\n"
- ", minimum size = %dKB\n", __func__, driver_pg1.host_diag_trace_max_size,
- driver_pg1.host_diag_trace_decrement_size,
- driver_pg1.host_diag_trace_min_size);
+ ", minimum size = %dKB\n", __func__,
+ le16_to_cpu(driver_pg1.host_diag_trace_max_size),
+ le16_to_cpu(driver_pg1.host_diag_trace_decrement_size),
+ le16_to_cpu(driver_pg1.host_diag_trace_min_size));
dprint_init(mrioc,
"%s:firmware diag buffer sizes read from driver\n"
"page1: maximum size = %dKB, decrement size = %dKB\n"
- ", minimum size = %dKB\n", __func__, driver_pg1.host_diag_fw_max_size,
- driver_pg1.host_diag_fw_decrement_size,
- driver_pg1.host_diag_fw_min_size);
+ ", minimum size = %dKB\n", __func__,
+ le16_to_cpu(driver_pg1.host_diag_fw_max_size),
+ le16_to_cpu(driver_pg1.host_diag_fw_decrement_size),
+ le16_to_cpu(driver_pg1.host_diag_fw_min_size));
if ((trace_size == 0) && (fw_size == 0))
return;
}
@@ -179,6 +181,12 @@ void mpi3mr_alloc_diag_bufs(struct mpi3mr_ioc *mrioc)
mpi3mr_alloc_trace_buffer(mrioc, trace_size)) {
retry = true;
+
+ if (!trace_dec_size || trace_dec_size > trace_size) {
+ retry = false;
+ goto retry_fw;
+ }
+
trace_size -= trace_dec_size;
dprint_init(mrioc, "trace diag buffer allocation failed\n"
"retrying smaller size %dKB\n", trace_size / 1024);
@@ -211,11 +219,13 @@ void mpi3mr_alloc_diag_bufs(struct mpi3mr_ioc *mrioc)
diag_buffer->size = fw_size;
} else {
retry = true;
- fw_size -= fw_dec_size;
- dprint_init(mrioc, "%s:trace diag buffer allocation failed,\n"
- "retrying smaller size %dKB\n",
- __func__, fw_size / 1024);
- goto retry_fw;
+ if (fw_dec_size && fw_dec_size <= fw_size) {
+ fw_size -= fw_dec_size;
+ dprint_init(mrioc, "%s:trace diag buffer allocation failed,\n"
+ "retrying smaller size %dKB\n",
+ __func__, fw_size / 1024);
+ goto retry_fw;
+ }
}
}
}
diff --git a/drivers/scsi/mpi3mr/mpi3mr_fw.c b/drivers/scsi/mpi3mr/mpi3mr_fw.c
index 31b19ed1528e..59241038f689 100644
--- a/drivers/scsi/mpi3mr/mpi3mr_fw.c
+++ b/drivers/scsi/mpi3mr/mpi3mr_fw.c
@@ -4107,26 +4107,35 @@ static int mpi3mr_repost_diag_bufs(struct mpi3mr_ioc *mrioc)
}
/**
- * mpi3mr_read_tsu_interval - Update time stamp interval
+ * mpi3mr_read_driver_page1 - Read Driver Page 1 parameters
* @mrioc: Adapter instance reference
*
- * Update time stamp interval if its defined in driver page 1,
- * otherwise use default value.
+ * Reads and caches Driver Page 1 parameters such as
+ * timestamp update interval and driver behavior flags.
*
* Return: Nothing
*/
static void
-mpi3mr_read_tsu_interval(struct mpi3mr_ioc *mrioc)
+mpi3mr_read_driver_page1(struct mpi3mr_ioc *mrioc)
{
struct mpi3_driver_page1 driver_pg1;
u16 pg_sz = sizeof(driver_pg1);
int retval = 0;
mrioc->ts_update_interval = MPI3MR_TSUPDATE_INTERVAL;
+ mrioc->skip_dev_shutdown_on_unload = 0;
retval = mpi3mr_cfg_get_driver_pg1(mrioc, &driver_pg1, pg_sz);
- if (!retval && driver_pg1.time_stamp_update)
+
+ if (retval)
+ return;
+
+ if (driver_pg1.time_stamp_update)
mrioc->ts_update_interval = (driver_pg1.time_stamp_update * 60);
+
+ mrioc->skip_dev_shutdown_on_unload =
+ (le32_to_cpu(driver_pg1.flags) &
+ MPI3_DRIVER1_FLAGS_DEVICE_SHUTDOWN_ON_UNLOAD_DISABLE) ? 1 : 0;
}
/**
@@ -4432,7 +4441,7 @@ int mpi3mr_init_ioc(struct mpi3mr_ioc *mrioc)
goto out_failed_noretry;
}
- mpi3mr_read_tsu_interval(mrioc);
+ mpi3mr_read_driver_page1(mrioc);
mpi3mr_print_ioc_info(mrioc);
dprint_init(mrioc, "allocating host diag buffers\n");
@@ -4604,7 +4613,7 @@ int mpi3mr_reinit_ioc(struct mpi3mr_ioc *mrioc, u8 is_resume)
goto out_failed_noretry;
}
- mpi3mr_read_tsu_interval(mrioc);
+ mpi3mr_read_driver_page1(mrioc);
mpi3mr_print_ioc_info(mrioc);
if (is_resume) {
@@ -5089,8 +5098,16 @@ static void mpi3mr_issue_ioc_shutdown(struct mpi3mr_ioc *mrioc)
return;
}
- shutdown_action = MPI3_SYSIF_IOC_CONFIG_SHUTDOWN_NORMAL |
- MPI3_SYSIF_IOC_CONFIG_DEVICE_SHUTDOWN_SEND_REQ;
+ shutdown_action = MPI3_SYSIF_IOC_CONFIG_SHUTDOWN_NORMAL;
+
+ if (!(mrioc->is_unload && mrioc->skip_dev_shutdown_on_unload))
+ shutdown_action |=
+ MPI3_SYSIF_IOC_CONFIG_DEVICE_SHUTDOWN_SEND_REQ;
+ else
+ ioc_info(mrioc,
+ "The shutdown request is issued without the device shutdown bit set\n"
+ "as indicated by the controller configuration\n");
+
ioc_config = readl(&mrioc->sysif_regs->ioc_configuration);
ioc_config |= shutdown_action;
diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
index 402d1f35d214..d2a20f2721db 100644
--- a/drivers/scsi/mpi3mr/mpi3mr_os.c
+++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
@@ -5665,6 +5665,8 @@ static void mpi3mr_remove(struct pci_dev *pdev)
return;
mrioc = shost_priv(shost);
+ mrioc->is_unload = true;
+
while (mrioc->reset_in_progress || mrioc->is_driver_loading)
ssleep(1);
--
2.47.3
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v4 01/10] mpi3mr: Skip device shutdown during unload per controller configuration
2026-08-05 11:06 ` [PATCH v4 01/10] mpi3mr: Skip device shutdown during unload per controller configuration Ranjan Kumar
@ 2026-08-05 11:33 ` sashiko-bot
0 siblings, 0 replies; 18+ messages in thread
From: sashiko-bot @ 2026-08-05 11:33 UTC (permalink / raw)
To: Ranjan Kumar; +Cc: linux-scsi
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [High] The driver rounds down the trace buffer size internally when allocating segmented buffer entries, but passes the original unrounded size to the firmware, which can cause the firmware to read past the end of the segment list and DMA write to arbitrary physical memory.
- [Medium] A trace buffer size smaller than 4KB results in a 0-byte DMA allocation, triggering a WARN_ON_ONCE within the kernel DMA API.
--
--- Patch [1]: [PATCH v4 01/10] mpi3mr: Skip device shutdown during unload per controller configuration ---
commit 9ac255a070f5df8da5fb46e54b2787eacb0b7c7c
Author: Ranjan Kumar <ranjan.kumar@broadcom.com>
mpi3mr: Skip device shutdown during unload per controller configuration
This commit caches Driver Page 1 settings to suppress device shutdown during
driver unload. It adds endianness conversions for the diagnostic buffer sizes
read from Driver Page 1 and hardens the allocation retry loops against invalid
decrement sizes provided by the firmware.
Closes: https://sashiko.dev/#/patchset/20260626114109.43685-1-ranjan.kumar@broadcom.com?part=1
Closes: https://sashiko.dev/#/patchset/20260708183305.244485-1-ranjan.kumar@broadcom.com?part=1
Closes: https://sashiko.dev/#/patchset/20260724102505.115136-1-ranjan.kumar@broadcom.com?part=1
> diff --git a/drivers/scsi/mpi3mr/mpi3mr_app.c b/drivers/scsi/mpi3mr/mpi3mr_app.c
> index 1353a8ff9c85d..fca5357a515d1 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_app.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_app.c
[ ... ]
> @@ -179,6 +181,12 @@ void mpi3mr_alloc_diag_bufs(struct mpi3mr_ioc *mrioc)
> mpi3mr_alloc_trace_buffer(mrioc, trace_size)) {
>
> retry = true;
> +
> + if (!trace_dec_size || trace_dec_size > trace_size) {
> + retry = false;
> + goto retry_fw;
> + }
> +
> trace_size -= trace_dec_size;
[Severity: High]
This is a pre-existing issue, but does this lead to firmware reading
out-of-bounds and potentially corrupting physical memory?
The driver rounds down the trace buffer size internally when allocating
segmented buffer entries in mpi3mr_alloc_trace_buffer():
seg_count = (trace_size) / MPI3MR_PAGE_SIZE_4K;
trace_size = seg_count * MPI3MR_PAGE_SIZE_4K;
However, it does not update the caller's trace_size. Thus, the original
unrounded trace_size is stored in mpi3mr_alloc_diag_bufs():
diag_buffer->size = trace_size;
When posting the buffer to firmware in mpi3mr_issue_diag_buf_post():
diag_buf_post_req.length = le32_to_cpu(diag_buffer->size);
The firmware calculates the required number of segments using the unrounded
size (e.g., ceil(5120/4096) = 2), but only 1 segment was allocated (size 8
bytes for the segment list). The firmware will read past the end of the
segment list (16 bytes), leading to an out-of-bounds DMA read. It will then
use the arbitrary out-of-bounds value as a physical address to DMA-write
trace data, leading to memory corruption.
[Severity: Medium]
This is a pre-existing issue, but can this retry loop cause a 0-byte DMA
allocation warning?
If the firmware-provided host_diag_trace_max_size (or the size after retry
decrements) is less than 4KB, trace_size will be less than 4096. When passed
to mpi3mr_alloc_trace_buffer(), it calculates seg_count = trace_size / 4096
which evaluates to 0.
This results in a 0-byte allocation in mpi3mr_alloc_trace_buffer():
diag_buffer_list = dma_alloc_coherent(&mrioc->pdev->dev,
sizeof(u64) * seg_count,
&diag_buffer_list_dma, GFP_KERNEL);
Calling the DMA API with a 0 size triggers a WARN_ON_ONCE(!size) splat in
dma_alloc_attrs(), which can crash systems with panic_on_warn enabled.
> dprint_init(mrioc, "trace diag buffer allocation failed\n"
> "retrying smaller size %dKB\n", trace_size / 1024);
> goto retry_trace;
> } else
> diag_buffer->size = trace_size;
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260805110634.346670-1-ranjan.kumar@broadcom.com?part=1
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v4 02/10] mpi3mr: Update MPI Headers to revision 41
2026-08-05 11:06 [PATCH v4 00/10] mpi3mr: Few Enhancements and minor fixes Ranjan Kumar
2026-08-05 11:06 ` [PATCH v4 01/10] mpi3mr: Skip device shutdown during unload per controller configuration Ranjan Kumar
@ 2026-08-05 11:06 ` Ranjan Kumar
2026-08-05 11:33 ` sashiko-bot
2026-08-05 11:06 ` [PATCH v4 03/10] mpi3mr: Add early timestamp synchronization after driver load Ranjan Kumar
` (7 subsequent siblings)
9 siblings, 1 reply; 18+ messages in thread
From: Ranjan Kumar @ 2026-08-05 11:06 UTC (permalink / raw)
To: linux-scsi, martin.petersen
Cc: sathya.prakash, chandrakanth.patil, vishakhavc, ipylypiv,
Ranjan Kumar
Update MPI Headers to revision 41
Signed-off-by: Ranjan Kumar <ranjan.kumar@broadcom.com>
---
drivers/scsi/mpi3mr/mpi/mpi30_cnfg.h | 77 +++++++++++++++++++++--
drivers/scsi/mpi3mr/mpi/mpi30_image.h | 7 ++-
drivers/scsi/mpi3mr/mpi/mpi30_ioc.h | 15 +++--
drivers/scsi/mpi3mr/mpi/mpi30_transport.h | 2 +-
4 files changed, 87 insertions(+), 14 deletions(-)
diff --git a/drivers/scsi/mpi3mr/mpi/mpi30_cnfg.h b/drivers/scsi/mpi3mr/mpi/mpi30_cnfg.h
index 33dd303c97bb..7cf16a5c15b7 100644
--- a/drivers/scsi/mpi3mr/mpi/mpi30_cnfg.h
+++ b/drivers/scsi/mpi3mr/mpi/mpi30_cnfg.h
@@ -72,6 +72,12 @@
#define MPI3_SECURITY_PGAD_SLOT_GROUP_SHIFT (8)
#define MPI3_SECURITY_PGAD_SLOT_MASK (0x000000ff)
#define MPI3_INSTANCE_PGAD_INSTANCE_MASK (0x0000ffff)
+#define MPI3_INSTANCE_PGAD_INSTANCE_SHIFT (0)
+#define MPI3_INTERFACE_PGAD_INTERFACE_MASK (0x0000000f)
+#define MPI3_INTERFACE_PGAD_INTERFACE_SHIFT (0)
+#define MPI3_INTERFACE_PGAD_INTERFACE_MPI (0)
+#define MPI3_INTERFACE_PGAD_INTERFACE_NVME_VD (1)
+#define MPI3_INTERFACE_PGAD_INTERFACE_NVME_PD (2)
struct mpi3_config_request {
__le16 host_tag;
u8 ioc_use_only02;
@@ -492,10 +498,31 @@ struct mpi3_man10_istwi_ctrlr_entry {
};
#define MPI3_MAN10_ISTWI_CTRLR_FLAGS_BUS_SPEED_MASK (0x000c)
-#define MPI3_MAN10_ISTWI_CTRLR_FLAGS_BUS_SPEED_100K (0x0000)
-#define MPI3_MAN10_ISTWI_CTRLR_FLAGS_BUS_SPEED_400K (0x0004)
+#define MPI3_MAN10_ISTWI_CTRLR_FLAGS_BUS_SPEED_100_KHZ (0x0000)
+#define MPI3_MAN10_ISTWI_CTRLR_FLAGS_BUS_SPEED_400_KHZ (0x0004)
#define MPI3_MAN10_ISTWI_CTRLR_FLAGS_TARGET_ENABLED (0x0002)
#define MPI3_MAN10_ISTWI_CTRLR_FLAGS_INITIATOR_ENABLED (0x0001)
+#define MPI3_MAN10_ISTWI_CTRLR_FLAGS_I2C_GLITCH_FLTR_MASK (0xc000)
+#define MPI3_MAN10_ISTWI_CTRLR_FLAGS_I2C_GLITCH_FLTR_SHIFT (14)
+#define MPI3_MAN10_ISTWI_CTRLR_FLAGS_I2C_GLITCH_FLTR_50_NS (0x0000)
+#define MPI3_MAN10_ISTWI_CTRLR_FLAGS_I2C_GLITCH_FLTR_10_NS (0x4000)
+#define MPI3_MAN10_ISTWI_CTRLR_FLAGS_I2C_GLITCH_FLTR_5_NS (0x8000)
+#define MPI3_MAN10_ISTWI_CTRLR_FLAGS_I2C_GLITCH_FLTR_0_NS (0xc000)
+#define MPI3_MAN10_ISTWI_CTRLR_FLAGS_BUS_TYPE_MASK (0x3000)
+#define MPI3_MAN10_ISTWI_CTRLR_FLAGS_BUS_TYPE_SHIFT (12)
+#define MPI3_MAN10_ISTWI_CTRLR_FLAGS_BUS_TYPE_I2C (0x0000)
+#define MPI3_MAN10_ISTWI_CTRLR_FLAGS_BUS_TYPE_I3C (0x1000)
+#define MPI3_MAN10_ISTWI_CTRLR_FLAGS_BUS_TYPE_AUTO (0x2000)
+#define MPI3_MAN10_ISTWI_CTRLR_FLAGS_I3C_MAX_DATA_RATE_MASK (0x0e00)
+#define MPI3_MAN10_ISTWI_CTRLR_FLAGS_I3C_MAX_DATA_RATE_SHIFT (9)
+#define MPI3_MAN10_ISTWI_CTRLR_FLAGS_I3C_MAX_DATA_RATE_12_5_MHZ (0x0000)
+#define MPI3_MAN10_ISTWI_CTRLR_FLAGS_I3C_MAX_DATA_RATE_8_MHZ (0x0200)
+#define MPI3_MAN10_ISTWI_CTRLR_FLAGS_I3C_MAX_DATA_RATE_6_MHZ (0x0400)
+#define MPI3_MAN10_ISTWI_CTRLR_FLAGS_I3C_MAX_DATA_RATE_4_MHZ (0x0600)
+#define MPI3_MAN10_ISTWI_CTRLR_FLAGS_I3C_MAX_DATA_RATE_2_MHZ (0x0800)
+#define MPI3_MAN10_ISTWI_CTRLR_FLAGS_BUS_SPEED_MASK (0x000c)
+#define MPI3_MAN10_ISTWI_CTRLR_FLAGS_BUS_SPEED_SHIFT (0)
+
#ifndef MPI3_MAN10_ISTWI_CTRLR_MAX
#define MPI3_MAN10_ISTWI_CTRLR_MAX (1)
#endif
@@ -1027,6 +1054,16 @@ struct mpi3_io_unit_page5 {
#define MPI3_IOUNIT5_DEVICE_SHUTDOWN_SWITCH_ATTACHED (0x02)
#define MPI3_IOUNIT5_DEVICE_SHUTDOWN_DIRECT_AND_EXPANDER (0x03)
#define MPI3_IOUNIT5_DEVICE_SHUTDOWN_DIRECT_AND_SWITCH (0x03)
+#define MPI3_IOUNIT5_DEVICE_SHUTDOWN_HDD_SPINDOWN_MASK (0xc000)
+#define MPI3_IOUNIT5_DEVICE_SHUTDOWN_HDD_SPINDOWN_NONE (0x0000)
+#define MPI3_IOUNIT5_DEVICE_SHUTDOWN_HDD_SPINDOWN_ALL (0x4000)
+#define MPI3_IOUNIT5_DEVICE_SHUTDOWN_HDD_SPINDOWN_FILTERED (0x8000)
+#define MPI3_IOUNIT5_DEVICE_SHUTDOWN_HDD_SPINDOWN_RESERVED (0xc000)
+#define MPI3_IOUNIT5_DEVICE_SHUTDOWN_SYNC_CACHE_MASK (0x3000)
+#define MPI3_IOUNIT5_DEVICE_SHUTDOWN_SYNC_CACHE_ALL (0x0000)
+#define MPI3_IOUNIT5_DEVICE_SHUTDOWN_SYNC_CACHE_FILTERED (0x1000)
+#define MPI3_IOUNIT5_DEVICE_SHUTDOWN_SYNC_CACHE_NONE (0x2000)
+#define MPI3_IOUNIT5_DEVICE_SHUTDOWN_SYNC_CACHE_RESERVED (0x3000)
#define MPI3_IOUNIT5_DEVICE_SHUTDOWN_SATA_HDD_MASK (0x0300)
#define MPI3_IOUNIT5_DEVICE_SHUTDOWN_SATA_HDD_SHIFT (8)
#define MPI3_IOUNIT5_DEVICE_SHUTDOWN_SAS_HDD_MASK (0x00c0)
@@ -1069,7 +1106,8 @@ struct mpi3_io_unit_page8 {
struct mpi3_config_page_header header;
u8 sb_mode;
u8 sb_state;
- __le16 reserved0a;
+ u8 flags;
+ u8 reserved0b;
u8 num_slots;
u8 slots_available;
u8 current_key_encryption_algo;
@@ -1088,6 +1126,8 @@ struct mpi3_io_unit_page8 {
#define MPI3_IOUNIT8_SBSTATE_SVN_UPDATE_PENDING (0x04)
#define MPI3_IOUNIT8_SBSTATE_KEY_UPDATE_PENDING (0x02)
#define MPI3_IOUNIT8_SBSTATE_SECURE_BOOT_ENABLED (0x01)
+#define MPI3_IOUNIT8_FLAGS_FWQR_CAPABLE (0x80)
+#define MPI3_IOUNIT8_FLAGS_FWQR_SECURED (0x40)
#define MPI3_IOUNIT8_SBMODE_CURRENT_KEY_IOUNIT17 (0x10)
#define MPI3_IOUNIT8_SBMODE_HARD_SECURE_RECERTIFIED (0x08)
struct mpi3_io_unit_page9 {
@@ -1174,10 +1214,16 @@ struct mpi3_io_unit_page12 {
#define MPI3_IOUNIT12_FLAGS_NUMPASSES_32 (0x00000200)
#define MPI3_IOUNIT12_FLAGS_NUMPASSES_64 (0x00000300)
#define MPI3_IOUNIT12_FLAGS_PASSPERIOD_MASK (0x00000003)
+#define MPI3_IOUNIT12_FLAGS_PASSPERIOD_SHIFT (0)
#define MPI3_IOUNIT12_FLAGS_PASSPERIOD_DISABLED (0x00000000)
#define MPI3_IOUNIT12_FLAGS_PASSPERIOD_500US (0x00000001)
#define MPI3_IOUNIT12_FLAGS_PASSPERIOD_1MS (0x00000002)
#define MPI3_IOUNIT12_FLAGS_PASSPERIOD_2MS (0x00000003)
+#define MPI3_IOUNIT12_FLAGS_INTERFACE_MASK (0x0000000c)
+#define MPI3_IOUNIT12_FLAGS_INTERFACE_SHIFT (2)
+#define MPI3_IOUNIT12_FLAGS_INTERFACE_MPI (0x00000000)
+#define MPI3_IOUNIT12_FLAGS_INTERFACE_NVME_VD (0x00000004)
+#define MPI3_IOUNIT12_FLAGS_INTERFACE_NVME_PD (0x00000008)
#ifndef MPI3_IOUNIT13_FUNC_MAX
#define MPI3_IOUNIT13_FUNC_MAX (1)
#endif
@@ -1238,6 +1284,7 @@ struct mpi3_io_unit_page15 {
#define MPI3_IOUNIT15_PAGEVERSION (0x00)
#define MPI3_IOUNIT15_FLAGS_EPRINIT_INITREQUIRED (0x04)
#define MPI3_IOUNIT15_FLAGS_EPRSUPPORT_MASK (0x03)
+#define MPI3_IOUNIT15_FLAGS_EPRSUPPORT_SHIFT (0)
#define MPI3_IOUNIT15_FLAGS_EPRSUPPORT_NOT_SUPPORTED (0x00)
#define MPI3_IOUNIT15_FLAGS_EPRSUPPORT_WITHOUT_POWER_BRAKE_GPIO (0x01)
#define MPI3_IOUNIT15_FLAGS_EPRSUPPORT_WITH_POWER_BRAKE_GPIO (0x02)
@@ -1255,6 +1302,9 @@ struct mpi3_io_unit_page17 {
__le32 current_key[];
};
#define MPI3_IOUNIT17_PAGEVERSION (0x00)
+#define MPI3_IOUNIT17_FLAGS_KEYROOT_MASK (0x01)
+#define MPI3_IOUNIT17_FLAGS_KEYROOT_HW (0x00)
+#define MPI3_IOUNIT17_FLAGS_KEYROOT_FW (0x01)
struct mpi3_io_unit_page18 {
struct mpi3_config_page_header header;
u8 flags;
@@ -1640,11 +1690,28 @@ struct mpi3_security_page3 {
};
#define MPI3_SECURITY3_PAGEVERSION (0x00)
-#define MPI3_SECURITY3_FLAGS_TYPE_MASK (0x0f)
+#define MPI3_SECURITY3_FLAGS_TYPE_MASK (0x1f)
#define MPI3_SECURITY3_FLAGS_TYPE_SHIFT (0)
#define MPI3_SECURITY3_FLAGS_TYPE_NOT_VALID (0)
#define MPI3_SECURITY3_FLAGS_TYPE_MLDSA_PRIVATE (1)
#define MPI3_SECURITY3_FLAGS_TYPE_MLDSA_PUBLIC (2)
+union mpi3_security_digest {
+ __le32 dword[16];
+ __le16 word[32];
+ u8 byte[64];
+};
+struct mpi3_security_page4 {
+ struct mpi3_config_page_header header;
+ __le32 reserved08[2];
+ union mpi3_security_mac mac;
+ union mpi3_security_nonce nonce;
+ u8 num_digests;
+ u8 hash_algorithm;
+ __le16 reserved92;
+ __le32 reserved94[3];
+ union mpi3_security_digest digest[];
+};
+#define MPI3_SECURITY4_PAGEVERSION (0x00)
struct mpi3_security_page10 {
struct mpi3_config_page_header header;
__le32 reserved08[2];
@@ -2074,7 +2141,7 @@ struct mpi3_sas_phy3_phy_event_config {
#define MPI3_SASPHY3_EVENT_CODE_LCCONN_TIME (0xd5)
#define MPI3_SASPHY3_EVENT_CODE_SSP_TX_START_TRANSMIT (0xd6)
#define MPI3_SASPHY3_EVENT_CODE_SATA_TX_START (0xd7)
-#define MPI3_SASPHY3_EVENT_CODE_SMP_TX_START_TRANSMT (0xd8)
+#define MPI3_SASPHY3_EVENT_CODE_SMP_TX_START_TRANSMIT (0xd8)
#define MPI3_SASPHY3_EVENT_CODE_TX_SMP_BREAK_CONN (0xd9)
#define MPI3_SASPHY3_EVENT_CODE_SSP_RX_START_RECEIVE (0xda)
#define MPI3_SASPHY3_EVENT_CODE_SATA_RX_START_RECEIVE (0xdb)
diff --git a/drivers/scsi/mpi3mr/mpi/mpi30_image.h b/drivers/scsi/mpi3mr/mpi/mpi30_image.h
index 62ddf094d46c..5fa09fa79358 100644
--- a/drivers/scsi/mpi3mr/mpi/mpi30_image.h
+++ b/drivers/scsi/mpi3mr/mpi/mpi30_image.h
@@ -18,7 +18,7 @@ struct mpi3_hash_exclusion_format {
__le32 size;
};
-#define MPI3_IMAGE_HASH_EXCUSION_NUM (4)
+#define MPI3_IMAGE_HASH_EXCLUSION_NUM (4)
struct mpi3_component_image_header {
__le32 signature0;
__le32 load_address;
@@ -42,7 +42,7 @@ struct mpi3_component_image_header {
union mpi3_version_union rmc_interface_version;
union mpi3_version_union etp_interface_version;
struct mpi3_comp_image_version component_image_version;
- struct mpi3_hash_exclusion_format hash_exclusion[MPI3_IMAGE_HASH_EXCUSION_NUM];
+ struct mpi3_hash_exclusion_format hash_exclusion[MPI3_IMAGE_HASH_EXCLUSION_NUM];
__le32 next_image_header_offset;
union mpi3_version_union security_version;
__le32 reserved84[31];
@@ -347,7 +347,8 @@ struct mpi3_encrypted_hash_entry {
struct mpi3_encrypted_hash_data {
u8 image_version;
u8 num_hash;
- __le16 reserved02;
+ u8 fw_num_hash;
+ u8 reserved03;
__le32 reserved04;
struct mpi3_encrypted_hash_entry encrypted_hash_entry[MPI3_ENCRYPTED_HASH_ENTRY_MAX];
};
diff --git a/drivers/scsi/mpi3mr/mpi/mpi30_ioc.h b/drivers/scsi/mpi3mr/mpi/mpi30_ioc.h
index 68efa0d51345..aa42fba7f930 100644
--- a/drivers/scsi/mpi3mr/mpi/mpi30_ioc.h
+++ b/drivers/scsi/mpi3mr/mpi/mpi30_ioc.h
@@ -428,10 +428,10 @@ struct mpi3_event_data_sas_discovery {
#define MPI3_EVENT_SAS_DISC_FLAGS_IN_PROGRESS (0x01)
#define MPI3_EVENT_SAS_DISC_RC_STARTED (0x01)
#define MPI3_EVENT_SAS_DISC_RC_COMPLETED (0x02)
-#define MPI3_SAS_DISC_STATUS_MAX_ENCLOSURES_EXCEED (0x80000000)
-#define MPI3_SAS_DISC_STATUS_MAX_EXPANDERS_EXCEED (0x40000000)
-#define MPI3_SAS_DISC_STATUS_MAX_DEVICES_EXCEED (0x20000000)
-#define MPI3_SAS_DISC_STATUS_MAX_TOPO_PHYS_EXCEED (0x10000000)
+#define MPI3_SAS_DISC_STATUS_MAX_ENCLOSURES_EXCEEDED (0x80000000)
+#define MPI3_SAS_DISC_STATUS_MAX_EXPANDERS_EXCEEDED (0x40000000)
+#define MPI3_SAS_DISC_STATUS_MAX_DEVICES_EXCEEDED (0x20000000)
+#define MPI3_SAS_DISC_STATUS_MAX_TOPO_PHYS_EXCEEDED (0x10000000)
#define MPI3_SAS_DISC_STATUS_INVALID_CEI (0x00010000)
#define MPI3_SAS_DISC_STATUS_FECEI_MISMATCH (0x00008000)
#define MPI3_SAS_DISC_STATUS_MULTIPLE_DEVICES_IN_SLOT (0x00004000)
@@ -965,7 +965,7 @@ struct mpi3_ci_download_reply {
u8 flags;
u8 cache_dirty;
u8 pending_count;
- u8 reserved13;
+ u8 additional_flags;
};
#define MPI3_CI_DOWNLOAD_FLAGS_DOWNLOAD_IN_PROGRESS (0x80)
@@ -979,6 +979,11 @@ struct mpi3_ci_download_reply {
#define MPI3_CI_DOWNLOAD_FLAGS_ACTIVATION_STATUS_ONLINE_PENDING (0x04)
#define MPI3_CI_DOWNLOAD_FLAGS_ACTIVATION_STATUS_OFFLINE_PENDING (0x06)
#define MPI3_CI_DOWNLOAD_FLAGS_COMPATIBLE (0x01)
+#define MPI3_CI_DOWNLOAD_ADDITIONALFLAGS_REDUNDANCYRESTORATION_MASK (0x03)
+#define MPI3_CI_DOWNLOAD_ADDITIONALFLAGS_REDUNDANCYRESTORATION_SHIFT (0)
+#define MPI3_CI_DOWNLOAD_ADDITIONALFLAGS_REDUNDANCYRESTORATION_NONE (0x00)
+#define MPI3_CI_DOWNLOAD_ADDITIONALFLAGS_REDUNDANCYRESTORATION_PRIMARY (0x01)
+#define MPI3_CI_DOWNLOAD_ADDITIONALFLAGS_REDUNDANCYRESTORATION_SECONDARY (0x02)
struct mpi3_ci_upload_request {
__le16 host_tag;
u8 ioc_use_only02;
diff --git a/drivers/scsi/mpi3mr/mpi/mpi30_transport.h b/drivers/scsi/mpi3mr/mpi/mpi30_transport.h
index 290a1f5c2924..794ecc778945 100644
--- a/drivers/scsi/mpi3mr/mpi/mpi30_transport.h
+++ b/drivers/scsi/mpi3mr/mpi/mpi30_transport.h
@@ -18,7 +18,7 @@ union mpi3_version_union {
#define MPI3_VERSION_MAJOR (3)
#define MPI3_VERSION_MINOR (0)
-#define MPI3_VERSION_UNIT (39)
+#define MPI3_VERSION_UNIT (41)
#define MPI3_VERSION_DEV (0)
#define MPI3_DEVHANDLE_INVALID (0xffff)
struct mpi3_sysif_oper_queue_indexes {
--
2.47.3
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v4 02/10] mpi3mr: Update MPI Headers to revision 41
2026-08-05 11:06 ` [PATCH v4 02/10] mpi3mr: Update MPI Headers to revision 41 Ranjan Kumar
@ 2026-08-05 11:33 ` sashiko-bot
0 siblings, 0 replies; 18+ messages in thread
From: sashiko-bot @ 2026-08-05 11:33 UTC (permalink / raw)
To: Ranjan Kumar; +Cc: linux-scsi
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Incorrect shift macro definition `MPI3_MAN10_ISTWI_CTRLR_FLAGS_BUS_SPEED_SHIFT` for mask `0x000c`.
--
--- Patch [2]: [PATCH v4 02/10] mpi3mr: Update MPI Headers to revision 41 ---
commit e227f986541dd8cdc45ad47d1ea7374c1b7cfc9c
Author: Ranjan Kumar <ranjan.kumar@broadcom.com>
mpi3mr: Update MPI Headers to revision 41
This commit updates the MPI headers to revision 41.
> diff --git a/drivers/scsi/mpi3mr/mpi/mpi30_cnfg.h b/drivers/scsi/mpi3mr/mpi/mpi30_cnfg.h
> index 33dd303c97bb5..7cf16a5c15b72 100644
> --- a/drivers/scsi/mpi3mr/mpi/mpi30_cnfg.h
> +++ b/drivers/scsi/mpi3mr/mpi/mpi30_cnfg.h
[ ... ]
> @@ -492,10 +498,31 @@ struct mpi3_man10_istwi_ctrlr_entry {
> };
>
> #define MPI3_MAN10_ISTWI_CTRLR_FLAGS_BUS_SPEED_MASK (0x000c)
> -#define MPI3_MAN10_ISTWI_CTRLR_FLAGS_BUS_SPEED_100K (0x0000)
> -#define MPI3_MAN10_ISTWI_CTRLR_FLAGS_BUS_SPEED_400K (0x0004)
> +#define MPI3_MAN10_ISTWI_CTRLR_FLAGS_BUS_SPEED_100_KHZ (0x0000)
> +#define MPI3_MAN10_ISTWI_CTRLR_FLAGS_BUS_SPEED_400_KHZ (0x0004)
> #define MPI3_MAN10_ISTWI_CTRLR_FLAGS_TARGET_ENABLED (0x0002)
> #define MPI3_MAN10_ISTWI_CTRLR_FLAGS_INITIATOR_ENABLED (0x0001)
> +#define MPI3_MAN10_ISTWI_CTRLR_FLAGS_I2C_GLITCH_FLTR_MASK (0xc000)
> +#define MPI3_MAN10_ISTWI_CTRLR_FLAGS_I2C_GLITCH_FLTR_SHIFT (14)
[ ... ]
> +#define MPI3_MAN10_ISTWI_CTRLR_FLAGS_I3C_MAX_DATA_RATE_2_MHZ (0x0800)
> +#define MPI3_MAN10_ISTWI_CTRLR_FLAGS_BUS_SPEED_MASK (0x000c)
> +#define MPI3_MAN10_ISTWI_CTRLR_FLAGS_BUS_SPEED_SHIFT (0)
[Severity: Low]
Is this shift value defined correctly?
The mask MPI3_MAN10_ISTWI_CTRLR_FLAGS_BUS_SPEED_MASK is defined as 0x000c,
which covers bits 2 and 3. Should MPI3_MAN10_ISTWI_CTRLR_FLAGS_BUS_SPEED_SHIFT
be 2 instead of 0 to ensure any future bitwise operations work properly?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260805110634.346670-1-ranjan.kumar@broadcom.com?part=2
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v4 03/10] mpi3mr: Add early timestamp synchronization after driver load
2026-08-05 11:06 [PATCH v4 00/10] mpi3mr: Few Enhancements and minor fixes Ranjan Kumar
2026-08-05 11:06 ` [PATCH v4 01/10] mpi3mr: Skip device shutdown during unload per controller configuration Ranjan Kumar
2026-08-05 11:06 ` [PATCH v4 02/10] mpi3mr: Update MPI Headers to revision 41 Ranjan Kumar
@ 2026-08-05 11:06 ` Ranjan Kumar
2026-08-05 11:06 ` [PATCH v4 04/10] mpi3mr: Fix NVMe page size caching for non-operational devices Ranjan Kumar
` (6 subsequent siblings)
9 siblings, 0 replies; 18+ messages in thread
From: Ranjan Kumar @ 2026-08-05 11:06 UTC (permalink / raw)
To: linux-scsi, martin.petersen
Cc: sathya.prakash, chandrakanth.patil, vishakhavc, ipylypiv,
Ranjan Kumar
When the driver is loaded from initramfs, the controller timestamp may
be initialized before the system clock has been synchronized. As a
result, the controller can operate with a stale timestamp until the
first periodic synchronization occurs.
Currently, the first controller timestamp synchronization occurs only
after the configured ts_update_interval expires (15 minutes by default).
Add an early timestamp synchronization 60 seconds after driver load,
followed by the existing periodic synchronization interval.
Signed-off-by: Ranjan Kumar <ranjan.kumar@broadcom.com>
---
drivers/scsi/mpi3mr/mpi3mr.h | 3 +++
drivers/scsi/mpi3mr/mpi3mr_fw.c | 25 +++++++++++++++++++------
2 files changed, 22 insertions(+), 6 deletions(-)
diff --git a/drivers/scsi/mpi3mr/mpi3mr.h b/drivers/scsi/mpi3mr/mpi3mr.h
index 39096004c60a..1f2f0951b560 100644
--- a/drivers/scsi/mpi3mr/mpi3mr.h
+++ b/drivers/scsi/mpi3mr/mpi3mr.h
@@ -125,6 +125,7 @@ extern atomic64_t event_counter;
#define MPI3MR_RESETTM_TIMEOUT 60
#define MPI3MR_RESET_HOST_IOWAIT_TIMEOUT 5
#define MPI3MR_TSUPDATE_INTERVAL 900
+#define MPI3MR_EARLY_TSUPDATE_SECONDS 60
#define MPI3MR_DEFAULT_SHUTDOWN_TIME 120
#define MPI3MR_RAID_ERRREC_RESET_TIMEOUT 180
#define MPI3MR_PREPARE_FOR_RESET_TIMEOUT 180
@@ -1118,6 +1119,7 @@ struct scmd_priv {
* @evtack_cmds_bitmap: Event Ack bitmap
* @delayed_evtack_cmds_list: Delayed event acknowledgment list
* @ts_update_counter: Timestamp update counter
+ * @early_ts_sync_done: Early (1 min) timestamp sync completed after load
* @ts_update_interval: Timestamp update interval
* @reset_in_progress: Reset in progress flag
* @unrecoverable: Controller unrecoverable flag
@@ -1318,6 +1320,7 @@ struct mpi3mr_ioc {
struct list_head delayed_evtack_cmds_list;
u16 ts_update_counter;
+ u8 early_ts_sync_done;
u16 ts_update_interval;
u8 reset_in_progress;
u8 unrecoverable;
diff --git a/drivers/scsi/mpi3mr/mpi3mr_fw.c b/drivers/scsi/mpi3mr/mpi3mr_fw.c
index 59241038f689..434b66f7b502 100644
--- a/drivers/scsi/mpi3mr/mpi3mr_fw.c
+++ b/drivers/scsi/mpi3mr/mpi3mr_fw.c
@@ -2870,8 +2870,9 @@ static int mpi3mr_print_pkg_ver(struct mpi3mr_ioc *mrioc)
* @work: work struct
*
* Watch dog work periodically executed (1 second interval) to
- * monitor firmware fault and to issue periodic timer sync to
- * the firmware.
+ * monitor firmware fault and perform timestamp synchronization
+ * to firmware, with an early sync 1 minute after load followed
+ * by periodic updates at ts_update_interval seconds (default 15 minutes).
*
* Return: Nothing.
*/
@@ -2917,11 +2918,23 @@ static void mpi3mr_watchdog_work(struct work_struct *work)
}
if (!(mrioc->facts.ioc_capabilities &
- MPI3_IOCFACTS_CAPABILITY_NON_SUPERVISOR_IOC) &&
- (mrioc->ts_update_counter++ >= mrioc->ts_update_interval)) {
+ MPI3_IOCFACTS_CAPABILITY_NON_SUPERVISOR_IOC)) {
+ if (!mrioc->early_ts_sync_done) {
+ /*
+ * Send time sync 1 min after load
+ */
+ if (mrioc->ts_update_counter++ >=
+ MPI3MR_EARLY_TSUPDATE_SECONDS) {
+ mrioc->early_ts_sync_done = 1;
+ mrioc->ts_update_counter = 0;
+ mpi3mr_sync_timestamp(mrioc);
+ }
+ } else if (mrioc->ts_update_counter++ >=
+ mrioc->ts_update_interval) {
+ mrioc->ts_update_counter = 0;
+ mpi3mr_sync_timestamp(mrioc);
+ }
- mrioc->ts_update_counter = 0;
- mpi3mr_sync_timestamp(mrioc);
}
if ((mrioc->prepare_for_reset) &&
--
2.47.3
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH v4 04/10] mpi3mr: Fix NVMe page size caching for non-operational devices
2026-08-05 11:06 [PATCH v4 00/10] mpi3mr: Few Enhancements and minor fixes Ranjan Kumar
` (2 preceding siblings ...)
2026-08-05 11:06 ` [PATCH v4 03/10] mpi3mr: Add early timestamp synchronization after driver load Ranjan Kumar
@ 2026-08-05 11:06 ` Ranjan Kumar
2026-08-05 11:40 ` sashiko-bot
2026-08-05 11:06 ` [PATCH v4 05/10] mpi3mr: Fix performance regression caused by extended IRQ poll sleep Ranjan Kumar
` (5 subsequent siblings)
9 siblings, 1 reply; 18+ messages in thread
From: Ranjan Kumar @ 2026-08-05 11:06 UTC (permalink / raw)
To: linux-scsi, martin.petersen
Cc: sathya.prakash, chandrakanth.patil, vishakhavc, ipylypiv,
Ranjan Kumar, Sashiko
For NVMe devices in an error state, the cached PCIe page size remains
unset. This causes management IOCTL validation to fail, preventing
requests from reaching firmware and returning incorrect errors to
userspace. Populate the page size attribute irrespective of
device access status so firmware can process IOCTLs and report
appropriate errors.
Additionally, harden the device initialization path against invalid
firmware data for non-operational devices:
1. Add bounds checking for page_size, falling back to 4096 bytes (shift
exponent 12) to prevent undefined shift behavior and kernel panics.
The minimum valid NVMe page size shift is 12 and maximum is 27.
2. Initialize reset_to and abort_to timeouts with default values to
prevent IOCTLs from failing instantly. To avoid race conditions
where concurrent readers might observe these default timeouts before
they are updated with firmware values, use local variables to
compute the final values before writing them to the device structure.
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/patchset/20260626114109.43685-1-ranjan.kumar@broadcom.com?part=4
Closes: https://sashiko.dev/#/patchset/20260708183305.244485-1-ranjan.kumar@broadcom.com?part=4
Closes: https://sashiko.dev/#/patchset/20260724102505.115136-1-ranjan.kumar@broadcom.com?part=4
Signed-off-by: Chandrakanth Patil <chandrakanth.patil@broadcom.com>
Signed-off-by: Ranjan Kumar <ranjan.kumar@broadcom.com>
---
drivers/scsi/mpi3mr/mpi3mr.h | 1 +
drivers/scsi/mpi3mr/mpi3mr_os.c | 24 ++++++++++++++++--------
2 files changed, 17 insertions(+), 8 deletions(-)
diff --git a/drivers/scsi/mpi3mr/mpi3mr.h b/drivers/scsi/mpi3mr/mpi3mr.h
index 1f2f0951b560..6128b30112e2 100644
--- a/drivers/scsi/mpi3mr/mpi3mr.h
+++ b/drivers/scsi/mpi3mr/mpi3mr.h
@@ -169,6 +169,7 @@ extern atomic64_t event_counter;
#define MPI3MR_DEFAULT_MDTS (128 * 1024)
#define MPI3MR_DEFAULT_PGSZEXP (12)
+#define MPI3MR_MAX_PGSZEXP (27)
/* Command retry count definitions */
#define MPI3MR_DEV_RMHS_RETRY_COUNT 3
diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
index d2a20f2721db..88b1d6360dac 100644
--- a/drivers/scsi/mpi3mr/mpi3mr_os.c
+++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
@@ -1349,24 +1349,32 @@ static void mpi3mr_update_tgtdev(struct mpi3mr_ioc *mrioc,
struct mpi3_device0_pcie_format *pcieinf =
&dev_pg0->device_specific.pcie_format;
u16 dev_info = le16_to_cpu(pcieinf->device_info);
+ u8 pgsz = MPI3MR_DEFAULT_PGSZEXP;
+ u8 reset_to = MPI3MR_INTADMCMD_TIMEOUT;
+ u8 abort_to = MPI3MR_INTADMCMD_TIMEOUT;
tgtdev->dev_spec.pcie_inf.dev_info = dev_info;
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 >= MPI3MR_DEFAULT_PGSZEXP &&
+ pcieinf->page_size <= MPI3MR_MAX_PGSZEXP)
+ pgsz = pcieinf->page_size;
+
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);
- tgtdev->dev_spec.pcie_inf.abort_to =
- max_t(u8, pcieinf->nvme_abort_to,
+ reset_to = max_t(u8, pcieinf->controller_reset_to,
+ MPI3MR_INTADMCMD_TIMEOUT);
+ abort_to = max_t(u8, pcieinf->nvme_abort_to,
MPI3MR_INTADMCMD_TIMEOUT);
}
+
+ tgtdev->dev_spec.pcie_inf.pgsz = pgsz;
+ tgtdev->dev_spec.pcie_inf.reset_to = reset_to;
+ tgtdev->dev_spec.pcie_inf.abort_to = abort_to;
+
if (tgtdev->dev_spec.pcie_inf.mdts > (1024 * 1024))
tgtdev->dev_spec.pcie_inf.mdts = (1024 * 1024);
if (((dev_info & MPI3_DEVICE0_PCIE_DEVICE_INFO_TYPE_MASK) !=
--
2.47.3
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v4 04/10] mpi3mr: Fix NVMe page size caching for non-operational devices
2026-08-05 11:06 ` [PATCH v4 04/10] mpi3mr: Fix NVMe page size caching for non-operational devices Ranjan Kumar
@ 2026-08-05 11:40 ` sashiko-bot
0 siblings, 0 replies; 18+ messages in thread
From: sashiko-bot @ 2026-08-05 11:40 UTC (permalink / raw)
To: Ranjan Kumar; +Cc: linux-scsi
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Time-of-Check to Time-of-Use (TOCTOU) / Double Fetch vulnerability in NVMe page size validation
--
--- Patch [4]: [PATCH v4 04/10] mpi3mr: Fix NVMe page size caching for non-operational devices ---
commit 84a34ae29dcd95f3e4be7fc74958e61313bd07c9
Author: Ranjan Kumar <ranjan.kumar@broadcom.com>
mpi3mr: Fix NVMe page size caching for non-operational devices
This commit populates the NVMe page size attribute irrespective of device
access status so firmware can process IOCTLs and report appropriate errors.
It also adds bounds checking for page_size to prevent undefined shift
behavior, and initializes timeout values via local variables to avoid race
conditions.
Closes: https://sashiko.dev/#/patchset/20260626114109.43685-1-ranjan.kumar@broadcom.com?part=4
Closes: https://sashiko.dev/#/patchset/20260708183305.244485-1-ranjan.kumar@broadcom.com?part=4
Closes: https://sashiko.dev/#/patchset/20260724102505.115136-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..88b1d6360dac2 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_os.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
[ ... ]
> @@ -1349,24 +1349,32 @@ static void mpi3mr_update_tgtdev(struct mpi3mr_ioc *mrioc,
> struct mpi3_device0_pcie_format *pcieinf =
> &dev_pg0->device_specific.pcie_format;
> u16 dev_info = le16_to_cpu(pcieinf->device_info);
> + u8 pgsz = MPI3MR_DEFAULT_PGSZEXP;
> + u8 reset_to = MPI3MR_INTADMCMD_TIMEOUT;
> + u8 abort_to = MPI3MR_INTADMCMD_TIMEOUT;
>
> tgtdev->dev_spec.pcie_inf.dev_info = dev_info;
> 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 >= MPI3MR_DEFAULT_PGSZEXP &&
> + pcieinf->page_size <= MPI3MR_MAX_PGSZEXP)
> + pgsz = pcieinf->page_size;
[Severity: High]
Could this introduce a time-of-check to time-of-use (TOCTOU) double fetch
vulnerability?
Since pcieinf points into dev_pg0, which resides directly in the DMA-mapped
reply buffer, is it possible for a compromised or malfunctioning PCIe device
to modify pcieinf->page_size in memory between the bounds check evaluation
and the final assignment?
If the device changes the value right after the check passes, an out-of-bounds
value might be stored into pgsz, bypassing the validation and potentially
triggering the undefined shift behavior this patch aims to prevent.
Would it be safer to fetch pcieinf->page_size into a local variable using
READ_ONCE() and then perform the validation and assignment on that local copy?
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260805110634.346670-1-ranjan.kumar@broadcom.com?part=4
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v4 05/10] mpi3mr: Fix performance regression caused by extended IRQ poll sleep
2026-08-05 11:06 [PATCH v4 00/10] mpi3mr: Few Enhancements and minor fixes Ranjan Kumar
` (3 preceding siblings ...)
2026-08-05 11:06 ` [PATCH v4 04/10] mpi3mr: Fix NVMe page size caching for non-operational devices Ranjan Kumar
@ 2026-08-05 11:06 ` Ranjan Kumar
2026-08-05 11:32 ` sashiko-bot
2026-08-05 11:06 ` [PATCH v4 06/10] mpi3mr: Fix memory leak on operational queue creation failure Ranjan Kumar
` (4 subsequent siblings)
9 siblings, 1 reply; 18+ messages in thread
From: Ranjan Kumar @ 2026-08-05 11:06 UTC (permalink / raw)
To: linux-scsi, martin.petersen
Cc: sathya.prakash, chandrakanth.patil, vishakhavc, ipylypiv,
Ranjan Kumar, Sashiko
Commit 24d7071d9645 ("scsi: mpi3mr: A performance fix") increased the
threaded IRQ poll sleep range from 2-20 us to 20-21 us to work around a
timer slack issue.
On kernels unaffected by the timer slack issue, the longer sleep interval
reduces reply queue processing efficiency and causes an approximately 7%
throughput regression on NVMe direct-attached RAID10 configurations.
Restore the IRQ poll sleep base to 2 us (widening the usleep_range() upper
bound to 10x the base instead of a fixed +1 us) to recover the lost
throughput, and skip the sleep entirely once pend_ios reaches 0 so the
poll loop exits immediately at the tail of a completion burst.
Additionally, resolve the following issues in the reply queue processing
and polling logic:
1. Add missing dma_rmb() memory barriers in the admin and operational
reply queue processing loops. This ensures that the descriptor
payload is only read after the phase bit check is complete, preventing
weakly ordered architectures from speculatively processing stale data.
2. Add bounds checking for `request_queue_id` in
mpi3mr_process_op_reply_q(). An out-of-range id is now logged and the
descriptor is retired (consumer index advanced, phase toggled on
wraparound, pend_ios/threshold accounted) rather than aborting the
loop in place, which previously left the same corrupted descriptor
at the head of the ring forever and stalled polling indefinitely.
3. Recheck for a late-arriving descriptor via dma_rmb() while still
holding op_reply_q->in_use, instead of releasing it and reclaiming
it afterward, which could race and reprocess a descriptor with
stale indices or double-decrement in_use.
4. Replace a direct panic() call with a safe ioc_err() log and abort in
mpi3mr_process_op_reply_desc() when mpi3mr_get_reply_virt_addr()
returns NULL. This prevents a single malformed DMA reply address from
crashing the entire host OS. The reply_dma output parameter is also
cleared before returning, since it was already populated with the
unvalidated address before the NULL check. Leaving it set would make
the caller repost that unvalidated address back to the hardware.
Note: The unbounded busy-wait loop (usleep_range) in mpi3mr_isr_poll()
flagged by automated review is intentionally retained. This short sleep
polling mechanism is critical for batching completions and achieving the
target throughput on high-performance NVMe configurations.
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/patchset/20260626114109.43685-1-ranjan.kumar@broadcom.com?part=5
Closes: https://sashiko.dev/#/patchset/20260708183305.244485-1-ranjan.kumar@broadcom.com?part=5
Closes: https://sashiko.dev/#/patchset/20260724102505.115136-1-ranjan.kumar@broadcom.com?part=5
Signed-off-by: Chandrakanth Patil <chandrakanth.patil@broadcom.com>
Signed-off-by: Ranjan Kumar <ranjan.kumar@broadcom.com>
---
drivers/scsi/mpi3mr/mpi3mr.h | 2 +-
drivers/scsi/mpi3mr/mpi3mr_fw.c | 52 ++++++++++++++++++++++++++++++---
drivers/scsi/mpi3mr/mpi3mr_os.c | 8 +++--
3 files changed, 55 insertions(+), 7 deletions(-)
diff --git a/drivers/scsi/mpi3mr/mpi3mr.h b/drivers/scsi/mpi3mr/mpi3mr.h
index 6128b30112e2..4d19a9460d38 100644
--- a/drivers/scsi/mpi3mr/mpi3mr.h
+++ b/drivers/scsi/mpi3mr/mpi3mr.h
@@ -179,7 +179,7 @@ extern atomic64_t event_counter;
#define MPI3MR_DEFAULT_SDEV_QD 32
/* Definitions for Threaded IRQ poll*/
-#define MPI3MR_IRQ_POLL_SLEEP 20
+#define MPI3MR_IRQ_POLL_SLEEP 2
#define MPI3MR_IRQ_POLL_TRIGGER_IOCOUNT 8
/* Definitions for the controller security status*/
diff --git a/drivers/scsi/mpi3mr/mpi3mr_fw.c b/drivers/scsi/mpi3mr/mpi3mr_fw.c
index 434b66f7b502..e6050b41e15a 100644
--- a/drivers/scsi/mpi3mr/mpi3mr_fw.c
+++ b/drivers/scsi/mpi3mr/mpi3mr_fw.c
@@ -473,6 +473,12 @@ int mpi3mr_process_admin_reply_q(struct mpi3mr_ioc *mrioc)
return 0;
}
+ /*
+ * Ensure that the descriptor payload is read only after
+ * the phase bit check is complete.
+ */
+ dma_rmb();
+
do {
if (mrioc->unrecoverable || mrioc->io_admin_reset_sync)
break;
@@ -493,6 +499,13 @@ int mpi3mr_process_admin_reply_q(struct mpi3mr_ioc *mrioc)
if ((le16_to_cpu(reply_desc->reply_flags) &
MPI3_REPLY_DESCRIPT_FLAGS_PHASE_MASK) != exp_phase)
break;
+
+ /*
+ * Ensure that the descriptor payload is read only after
+ * the phase bit check is complete.
+ */
+ dma_rmb();
+
if (threshold_comps == MPI3MR_THRESHOLD_REPLY_COUNT) {
writel(admin_reply_ci,
&mrioc->sysif_regs->admin_reply_queue_ci);
@@ -564,15 +577,33 @@ int mpi3mr_process_op_reply_q(struct mpi3mr_ioc *mrioc,
reply_desc = mpi3mr_get_reply_desc(op_reply_q, reply_ci);
if ((le16_to_cpu(reply_desc->reply_flags) &
MPI3_REPLY_DESCRIPT_FLAGS_PHASE_MASK) != exp_phase) {
+ /* Recheck under in_use before releasing, to avoid a reclaim race */
+ dma_rmb();
+ if ((le16_to_cpu(reply_desc->reply_flags) &
+ MPI3_REPLY_DESCRIPT_FLAGS_PHASE_MASK) == exp_phase)
+ goto process_desc;
atomic_dec(&op_reply_q->in_use);
return 0;
}
+process_desc:
+ /*
+ * Ensure that the descriptor payload is read only after
+ * the phase bit check is complete.
+ */
+ dma_rmb();
do {
if (mrioc->unrecoverable || mrioc->io_admin_reset_sync)
break;
req_q_idx = le16_to_cpu(reply_desc->request_queue_id) - 1;
+
+ if (unlikely(req_q_idx >= mrioc->num_op_req_q)) {
+ ioc_err(mrioc, "Invalid request queue id %d, skipping reply\n",
+ req_q_idx + 1);
+ goto next_reply;
+ }
+
op_req_q = &mrioc->req_qinfo[req_q_idx];
WRITE_ONCE(op_req_q->ci, le16_to_cpu(reply_desc->request_queue_ci));
@@ -581,6 +612,7 @@ int mpi3mr_process_op_reply_q(struct mpi3mr_ioc *mrioc,
if (reply_dma)
mpi3mr_repost_reply_buf(mrioc, reply_dma);
+next_reply:
num_op_reply++;
threshold_comps++;
@@ -592,8 +624,19 @@ int mpi3mr_process_op_reply_q(struct mpi3mr_ioc *mrioc,
reply_desc = mpi3mr_get_reply_desc(op_reply_q, reply_ci);
if ((le16_to_cpu(reply_desc->reply_flags) &
- MPI3_REPLY_DESCRIPT_FLAGS_PHASE_MASK) != exp_phase)
+ MPI3_REPLY_DESCRIPT_FLAGS_PHASE_MASK) != exp_phase) {
+ dma_rmb();
+ if ((le16_to_cpu(reply_desc->reply_flags) &
+ MPI3_REPLY_DESCRIPT_FLAGS_PHASE_MASK) == exp_phase)
+ goto reply_ready;
break;
+ }
+reply_ready:
+ /*
+ * Ensure that the descriptor payload is read only after
+ * the phase bit check is complete.
+ */
+ dma_rmb();
#ifndef CONFIG_PREEMPT_RT
/*
* Exit completion loop to avoid CPU lockup
@@ -743,11 +786,12 @@ static irqreturn_t mpi3mr_isr_poll(int irq, void *privdata)
num_op_reply +=
mpi3mr_process_op_reply_q(mrioc,
intr_info->op_reply_q);
+ if (!atomic_read(&intr_info->op_reply_q->pend_ios))
+ break;
- usleep_range(MPI3MR_IRQ_POLL_SLEEP, MPI3MR_IRQ_POLL_SLEEP + 1);
+ usleep_range(MPI3MR_IRQ_POLL_SLEEP, 10 * MPI3MR_IRQ_POLL_SLEEP);
- } while (atomic_read(&intr_info->op_reply_q->pend_ios) &&
- (num_op_reply < mrioc->max_host_ios));
+ } while (num_op_reply < mrioc->max_host_ios);
intr_info->op_reply_q->enable_irq_poll = false;
enable_irq(intr_info->os_irq);
diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
index 88b1d6360dac..23a6a5e3df5f 100644
--- a/drivers/scsi/mpi3mr/mpi3mr_os.c
+++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
@@ -3430,8 +3430,12 @@ void mpi3mr_process_op_reply_desc(struct mpi3mr_ioc *mrioc,
scsi_reply = mpi3mr_get_reply_virt_addr(mrioc,
*reply_dma);
if (!scsi_reply) {
- panic("%s: scsi_reply is NULL, this shouldn't happen\n",
- mrioc->name);
+ ioc_err(mrioc, "scsi_reply is NULL, invalid reply_frame_address\n");
+ /*
+ * Do not let the caller repost an address that
+ * failed virt-addr lookup back to the hardware.
+ */
+ *reply_dma = 0;
goto out;
}
host_tag = le16_to_cpu(scsi_reply->host_tag);
--
2.47.3
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v4 05/10] mpi3mr: Fix performance regression caused by extended IRQ poll sleep
2026-08-05 11:06 ` [PATCH v4 05/10] mpi3mr: Fix performance regression caused by extended IRQ poll sleep Ranjan Kumar
@ 2026-08-05 11:32 ` sashiko-bot
0 siblings, 0 replies; 18+ messages in thread
From: sashiko-bot @ 2026-08-05 11:32 UTC (permalink / raw)
To: Ranjan Kumar; +Cc: linux-scsi
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues:
- [High] The threaded IRQ polling loop will loop indefinitely if `pend_ios` becomes negative due to skipped corrupt descriptors.
Pre-existing issues:
- [High] The admin reply queue path reposts unvalidated DMA addresses to the hardware free queue when virtual address lookup fails, causing host memory corruption.
- [High] Missing release memory barrier when unlocking the `in_use` queue lock, allowing race conditions on weakly ordered architectures.
--
--- Patch [5]: [PATCH v4 05/10] mpi3mr: Fix performance regression caused by extended IRQ poll sleep ---
commit 19ca49ca3a484c6837fd5a670f8512d409c2678d
Author: Ranjan Kumar <ranjan.kumar@broadcom.com>
mpi3mr: Fix performance regression caused by extended IRQ poll sleep
This patch restores the IRQ poll sleep base to 2 us and introduces bounds
checking for request queue IDs to prevent infinite polling. It also adds
memory barriers in the admin and operational reply queue processing loops
and fixes a panic condition on malformed DMA reply addresses.
Link: https://sashiko.dev/#/patchset/20260626114109.43685-1-ranjan.kumar@broadcom.com?part=5
Link: https://sashiko.dev/#/patchset/20260708183305.244485-1-ranjan.kumar@broadcom.com?part=5
Link: https://sashiko.dev/#/patchset/20260724102505.115136-1-ranjan.kumar@broadcom.com?part=5
> diff --git a/drivers/scsi/mpi3mr/mpi3mr_fw.c b/drivers/scsi/mpi3mr/mpi3mr_fw.c
> --- a/drivers/scsi/mpi3mr/mpi3mr_fw.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_fw.c
[ ... ]
> @@ -565,15 +578,33 @@ int mpi3mr_process_op_reply_q(struct mpi3mr_ioc *mrioc,
> reply_desc = mpi3mr_get_reply_desc(op_reply_q, reply_ci);
> if ((le16_to_cpu(reply_desc->reply_flags) &
> MPI3_REPLY_DESCRIPT_FLAGS_PHASE_MASK) != exp_phase) {
> + /* Recheck under in_use before releasing, to avoid a reclaim race */
> + dma_rmb();
> + if ((le16_to_cpu(reply_desc->reply_flags) &
> + MPI3_REPLY_DESCRIPT_FLAGS_PHASE_MASK) == exp_phase)
> + goto process_desc;
> atomic_dec(&op_reply_q->in_use);
[Severity: High]
This is a pre-existing issue, but is a release memory barrier missing when
unlocking the in_use queue lock?
Both here and at the end of mpi3mr_process_op_reply_q(), the lock state is
released via atomic_dec():
op_reply_q->ci = reply_ci;
op_reply_q->ephase = exp_phase;
atomic_sub(threshold_comps, &op_reply_q->pend_ios);
atomic_dec(&op_reply_q->in_use);
Since non-value-returning RMW atomic operations like atomic_dec() do not
imply memory barriers, could weakly ordered architectures reorder the writes
to op_reply_q->ci and op_reply_q->ephase after the in_use decrement?
Could this allow another CPU to acquire in_use but read stale values for ci
and ephase, causing it to double-process reply descriptors?
> return 0;
> }
[ ... ]
> @@ -744,11 +787,12 @@ static irqreturn_t mpi3mr_isr_poll(int irq, void *privdata)
> num_op_reply +=
> mpi3mr_process_op_reply_q(mrioc,
> intr_info->op_reply_q);
> + if (!atomic_read(&intr_info->op_reply_q->pend_ios))
> + break;
[Severity: High]
Can pend_ios become negative and cause this loop to run indefinitely?
If mpi3mr_process_op_reply_q() encounters a corrupted descriptor:
if (unlikely(req_q_idx >= mrioc->num_op_req_q)) {
ioc_err(mrioc, "Invalid request queue id %d, skipping reply\n",
req_q_idx + 1);
goto next_reply;
}
It skips the reply but still increments threshold_comps, which is later
subtracted from pend_ios. If the descriptor did not correspond to a tracked
host submission, couldn't this decrement pend_ios below zero?
Since this exact equality check (!atomic_read) evaluates to false for
negative values, will the polling loop fail to break and wait indefinitely?
> - usleep_range(MPI3MR_IRQ_POLL_SLEEP, MPI3MR_IRQ_POLL_SLEEP + 1);
> + usleep_range(MPI3MR_IRQ_POLL_SLEEP, 10 * MPI3MR_IRQ_POLL_SLEEP);
>
> - } while (atomic_read(&intr_info->op_reply_q->pend_ios) &&
> - (num_op_reply < mrioc->max_host_ios));
> + } while (num_op_reply < mrioc->max_host_ios);
>
> diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
> --- a/drivers/scsi/mpi3mr/mpi3mr_os.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
> @@ -3430,8 +3430,12 @@ void mpi3mr_process_op_reply_desc(struct mpi3mr_ioc *mrioc,
> scsi_reply = mpi3mr_get_reply_virt_addr(mrioc,
> *reply_dma);
> if (!scsi_reply) {
> - panic("%s: scsi_reply is NULL, this shouldn't happen\n",
> - mrioc->name);
> + ioc_err(mrioc, "scsi_reply is NULL, invalid reply_frame_address\n");
> + /*
> + * Do not let the caller repost an address that
> + * failed virt-addr lookup back to the hardware.
> + */
> + *reply_dma = 0;
[Severity: High]
This isn't a bug introduced by this patch, but does the admin reply queue
path suffer from the same unvalidated DMA address issue fixed here for the
operational queue?
In mpi3mr_process_admin_reply_desc():
def_reply = mpi3mr_get_reply_virt_addr(mrioc, *reply_dma);
if (!def_reply)
goto out;
If def_reply is NULL, the function jumps to the out label but fails to
clear *reply_dma. Then in mpi3mr_process_admin_reply_q():
mpi3mr_process_admin_reply_desc(mrioc, reply_desc, &reply_dma);
if (reply_dma)
mpi3mr_repost_reply_buf(mrioc, reply_dma);
Will this cause the caller to unknowingly repost the unvalidated, non-zero
reply_dma back to the hardware free queue, leading to host memory
corruption?
> goto out;
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260805110634.346670-1-ranjan.kumar@broadcom.com?part=5
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v4 06/10] mpi3mr: Fix memory leak on operational queue creation failure
2026-08-05 11:06 [PATCH v4 00/10] mpi3mr: Few Enhancements and minor fixes Ranjan Kumar
` (4 preceding siblings ...)
2026-08-05 11:06 ` [PATCH v4 05/10] mpi3mr: Fix performance regression caused by extended IRQ poll sleep Ranjan Kumar
@ 2026-08-05 11:06 ` Ranjan Kumar
2026-08-05 11:40 ` sashiko-bot
2026-08-05 11:06 ` [PATCH v4 07/10] mpi3mr: Fix firmware event reference leak during cleanup Ranjan Kumar
` (3 subsequent siblings)
9 siblings, 1 reply; 18+ messages in thread
From: Ranjan Kumar @ 2026-08-05 11:06 UTC (permalink / raw)
To: linux-scsi, martin.petersen
Cc: sathya.prakash, chandrakanth.patil, vishakhavc, ipylypiv,
Ranjan Kumar, Sashiko
When operational queue creation fails after one or more queues have
been created, the error path frees the queue information arrays but
does not release the DMA memory segments associated with the created
queues, resulting in a memory leak. Fix this by ensuring that partially
allocated segments are freed immediately if a queue fails to create.
Additionally, resolve the following issues in the queue segment
free/alloc paths:
1. Clear mrioc->intr_info[].op_reply_q with WRITE_ONCE() and follow it
with synchronize_irq() before freeing segments, and have the ISR
paths read it once via READ_ONCE() into a local, to close a race
where the ISR could use the pointer while it is being freed.
2. Free q_segment_list before checking q_segments in both free
functions, since a kzalloc_objs() failure on q_segments left
q_segment_list leaked via the early return.
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/patchset/20260626114109.43685-1-ranjan.kumar@broadcom.com?part=6
Closes: https://sashiko.dev/#/patchset/20260708183305.244485-1-ranjan.kumar@broadcom.com?part=6
Closes: https://sashiko.dev/#/patchset/20260724102505.115136-1-ranjan.kumar@broadcom.com?part=6
Signed-off-by: Chandrakanth Patil <chandrakanth.patil@broadcom.com>
Signed-off-by: Ranjan Kumar <ranjan.kumar@broadcom.com>
---
drivers/scsi/mpi3mr/mpi3mr_fw.c | 83 ++++++++++++++++++++++++---------
1 file changed, 60 insertions(+), 23 deletions(-)
diff --git a/drivers/scsi/mpi3mr/mpi3mr_fw.c b/drivers/scsi/mpi3mr/mpi3mr_fw.c
index e6050b41e15a..5a780eb7fd43 100644
--- a/drivers/scsi/mpi3mr/mpi3mr_fw.c
+++ b/drivers/scsi/mpi3mr/mpi3mr_fw.c
@@ -698,6 +698,7 @@ static irqreturn_t mpi3mr_isr_primary(int irq, void *privdata)
{
struct mpi3mr_intr_info *intr_info = privdata;
struct mpi3mr_ioc *mrioc;
+ struct op_reply_qinfo *op_reply_q;
u16 midx;
u32 num_admin_replies = 0, num_op_reply = 0;
@@ -713,9 +714,9 @@ static irqreturn_t mpi3mr_isr_primary(int irq, void *privdata)
if (!midx)
num_admin_replies = mpi3mr_process_admin_reply_q(mrioc);
- if (intr_info->op_reply_q)
- num_op_reply = mpi3mr_process_op_reply_q(mrioc,
- intr_info->op_reply_q);
+ op_reply_q = READ_ONCE(intr_info->op_reply_q);
+ if (op_reply_q)
+ num_op_reply = mpi3mr_process_op_reply_q(mrioc, op_reply_q);
if (num_admin_replies || num_op_reply)
return IRQ_HANDLED;
@@ -728,6 +729,7 @@ static irqreturn_t mpi3mr_isr_primary(int irq, void *privdata)
static irqreturn_t mpi3mr_isr(int irq, void *privdata)
{
struct mpi3mr_intr_info *intr_info = privdata;
+ struct op_reply_qinfo *op_reply_q;
int ret;
if (!intr_info)
@@ -740,11 +742,12 @@ static irqreturn_t mpi3mr_isr(int irq, void *privdata)
* If more IOs are expected, schedule IRQ polling thread.
* Otherwise exit from ISR.
*/
- if ((threaded_isr_poll == false) || !intr_info->op_reply_q)
+ op_reply_q = READ_ONCE(intr_info->op_reply_q);
+ if ((threaded_isr_poll == false) || !op_reply_q)
return ret;
- if (!intr_info->op_reply_q->enable_irq_poll ||
- !atomic_read(&intr_info->op_reply_q->pend_ios))
+ if (!op_reply_q->enable_irq_poll ||
+ !atomic_read(&op_reply_q->pend_ios))
return ret;
disable_irq_nosync(intr_info->os_irq);
@@ -766,10 +769,15 @@ static irqreturn_t mpi3mr_isr_poll(int irq, void *privdata)
{
struct mpi3mr_intr_info *intr_info = privdata;
struct mpi3mr_ioc *mrioc;
+ struct op_reply_qinfo *op_reply_q;
u16 midx;
u32 num_op_reply = 0;
- if (!intr_info || !intr_info->op_reply_q)
+ if (!intr_info)
+ return IRQ_NONE;
+
+ op_reply_q = READ_ONCE(intr_info->op_reply_q);
+ if (!op_reply_q)
return IRQ_NONE;
mrioc = intr_info->mrioc;
@@ -780,20 +788,23 @@ static irqreturn_t mpi3mr_isr_poll(int irq, void *privdata)
if (!mrioc->intr_enabled || mrioc->unrecoverable)
break;
+ op_reply_q = READ_ONCE(intr_info->op_reply_q);
+ if (!op_reply_q)
+ break;
+
if (!midx)
mpi3mr_process_admin_reply_q(mrioc);
- if (intr_info->op_reply_q)
- num_op_reply +=
- mpi3mr_process_op_reply_q(mrioc,
- intr_info->op_reply_q);
- if (!atomic_read(&intr_info->op_reply_q->pend_ios))
+ num_op_reply +=
+ mpi3mr_process_op_reply_q(mrioc, op_reply_q);
+ if (!atomic_read(&op_reply_q->pend_ios))
break;
usleep_range(MPI3MR_IRQ_POLL_SLEEP, 10 * MPI3MR_IRQ_POLL_SLEEP);
} while (num_op_reply < mrioc->max_host_ios);
- intr_info->op_reply_q->enable_irq_poll = false;
+ if (op_reply_q)
+ op_reply_q->enable_irq_poll = false;
enable_irq(intr_info->os_irq);
return IRQ_HANDLED;
@@ -1973,10 +1984,6 @@ static void mpi3mr_free_op_req_q_segments(struct mpi3mr_ioc *mrioc, u16 q_idx)
int size;
struct segments *segments;
- segments = mrioc->req_qinfo[q_idx].q_segments;
- if (!segments)
- return;
-
if (mrioc->enable_segqueue) {
size = MPI3MR_OP_REQ_Q_SEG_SIZE;
if (mrioc->req_qinfo[q_idx].q_segment_list) {
@@ -1990,6 +1997,10 @@ static void mpi3mr_free_op_req_q_segments(struct mpi3mr_ioc *mrioc, u16 q_idx)
size = mrioc->req_qinfo[q_idx].segment_qd *
mrioc->facts.op_req_sz;
+ segments = mrioc->req_qinfo[q_idx].q_segments;
+ if (!segments)
+ return;
+
for (j = 0; j < mrioc->req_qinfo[q_idx].num_segments; j++) {
if (!segments[j].segment)
continue;
@@ -2016,10 +2027,17 @@ static void mpi3mr_free_op_reply_q_segments(struct mpi3mr_ioc *mrioc, u16 q_idx)
u16 j;
int size;
struct segments *segments;
+ u16 midx = REPLY_QUEUE_IDX_TO_MSIX_IDX(q_idx, mrioc->op_reply_q_offset);
- segments = mrioc->op_reply_qinfo[q_idx].q_segments;
- if (!segments)
- return;
+ /*
+ * Stop the ISR/poll thread from picking up this queue before its
+ * segments are freed below, and wait for any in-flight handler
+ * that already has the old pointer to finish using it.
+ */
+ if (midx < mrioc->intr_info_count) {
+ WRITE_ONCE(mrioc->intr_info[midx].op_reply_q, NULL);
+ synchronize_irq(pci_irq_vector(mrioc->pdev, midx));
+ }
if (mrioc->enable_segqueue) {
size = MPI3MR_OP_REP_Q_SEG_SIZE;
@@ -2034,6 +2052,10 @@ static void mpi3mr_free_op_reply_q_segments(struct mpi3mr_ioc *mrioc, u16 q_idx)
size = mrioc->op_reply_qinfo[q_idx].segment_qd *
mrioc->op_reply_desc_sz;
+ segments = mrioc->op_reply_qinfo[q_idx].q_segments;
+ if (!segments)
+ return;
+
for (j = 0; j < mrioc->op_reply_qinfo[q_idx].num_segments; j++) {
if (!segments[j].segment)
continue;
@@ -2500,7 +2522,7 @@ static int mpi3mr_create_op_req_q(struct mpi3mr_ioc *mrioc, u16 idx,
static int mpi3mr_create_op_queues(struct mpi3mr_ioc *mrioc)
{
int retval = 0;
- u16 num_queues = 0, i = 0, msix_count_op_q = 1;
+ u16 num_queues = 0, i = 0, j = 0, msix_count_op_q = 1;
u32 ioc_status;
enum mpi3mr_iocstate ioc_state;
@@ -2552,6 +2574,13 @@ static int mpi3mr_create_op_queues(struct mpi3mr_ioc *mrioc)
}
}
+ if (i < num_queues) {
+ for (j = i; j < num_queues; j++) {
+ mpi3mr_free_op_req_q_segments(mrioc, j);
+ mpi3mr_free_op_reply_q_segments(mrioc, j);
+ }
+ }
+
if (i == 0) {
/* Not even one queue is created successfully*/
retval = -1;
@@ -2573,11 +2602,19 @@ static int mpi3mr_create_op_queues(struct mpi3mr_ioc *mrioc)
return retval;
out_failed:
- kfree(mrioc->req_qinfo);
- mrioc->req_qinfo = NULL;
+ if (mrioc->req_qinfo) {
+ for (j = 0; j < i; j++) {
+ mpi3mr_free_op_req_q_segments(mrioc, j);
+ mpi3mr_free_op_reply_q_segments(mrioc, j);
+ }
+ kfree(mrioc->req_qinfo);
+ mrioc->req_qinfo = NULL;
+ }
+ mrioc->num_op_req_q = 0;
kfree(mrioc->op_reply_qinfo);
mrioc->op_reply_qinfo = NULL;
+ mrioc->num_op_reply_q = 0;
return retval;
}
--
2.47.3
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v4 06/10] mpi3mr: Fix memory leak on operational queue creation failure
2026-08-05 11:06 ` [PATCH v4 06/10] mpi3mr: Fix memory leak on operational queue creation failure Ranjan Kumar
@ 2026-08-05 11:40 ` sashiko-bot
0 siblings, 0 replies; 18+ messages in thread
From: sashiko-bot @ 2026-08-05 11:40 UTC (permalink / raw)
To: Ranjan Kumar; +Cc: linux-scsi
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The threaded IRQ handler `mpi3mr_isr_poll` may exit early without re-enabling the IRQ, leaving the interrupt line permanently disabled.
- [High] Polling paths pass a potentially NULL `op_reply_q` pointer to `mpi3mr_process_op_reply_q`, leading to a kernel panic.
--
--- Patch [6]: [PATCH v4 06/10] mpi3mr: Fix memory leak on operational queue creation failure ---
commit 3a3eabe2f05c3500da7a32007ea4df206d106e35
Author: Ranjan Kumar <ranjan.kumar@broadcom.com>
mpi3mr: Fix memory leak on operational queue creation failure
When operational queue creation fails, the error path frees queue information
arrays but does not release DMA memory segments. This patch ensures partially
allocated segments are freed immediately. It also attempts to resolve races
in the queue segment free/alloc paths by clearing the queue pointer before
freeing.
Closes: https://sashiko.dev/#/patchset/20260626114109.43685-1-ranjan.kumar@broadcom.com?part=6
Closes: https://sashiko.dev/#/patchset/20260708183305.244485-1-ranjan.kumar@broadcom.com?part=6
Closes: https://sashiko.dev/#/patchset/20260724102505.115136-1-ranjan.kumar@broadcom.com?part=6
> diff --git a/drivers/scsi/mpi3mr/mpi3mr_fw.c b/drivers/scsi/mpi3mr/mpi3mr_fw.c
> index e469507045dd6..880c07bdc9738 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_fw.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_fw.c
[ ... ]
> @@ -767,10 +770,15 @@ static irqreturn_t mpi3mr_isr_poll(int irq, void *privdata)
> {
> struct mpi3mr_intr_info *intr_info = privdata;
> struct mpi3mr_ioc *mrioc;
> + struct op_reply_qinfo *op_reply_q;
> u16 midx;
> u32 num_op_reply = 0;
>
> - if (!intr_info || !intr_info->op_reply_q)
> + if (!intr_info)
> + return IRQ_NONE;
> +
> + op_reply_q = READ_ONCE(intr_info->op_reply_q);
> + if (!op_reply_q)
> return IRQ_NONE;
[Severity: High]
Does returning early here leave the interrupt permanently masked?
If a concurrent queue deletion (e.g., from a reset) sets op_reply_q to NULL
right after mpi3mr_isr() reads it as non-NULL, the hardirq handler will
disable the IRQ and schedule this threaded handler:
mpi3mr_isr()
disable_irq_nosync(intr_info->os_irq);
return IRQ_WAKE_THREAD;
When mpi3mr_isr_poll() runs, it will read op_reply_q as NULL and return
IRQ_NONE without calling enable_irq(intr_info->os_irq). Future IOs on the
operational queue associated with this vector would hang.
>
> mrioc = intr_info->mrioc;
[ ... ]
> @@ -2021,10 +2032,17 @@ static void mpi3mr_free_op_reply_q_segments(struct mpi3mr_ioc *mrioc, u16 q_idx)
> u16 j;
> int size;
> struct segments *segments;
> + u16 midx = REPLY_QUEUE_IDX_TO_MSIX_IDX(q_idx, mrioc->op_reply_q_offset);
>
> - segments = mrioc->op_reply_qinfo[q_idx].q_segments;
> - if (!segments)
> - return;
> + /*
> + * Stop the ISR/poll thread from picking up this queue before its
> + * segments are freed below, and wait for any in-flight handler
> + * that already has the old pointer to finish using it.
> + */
> + if (midx < mrioc->intr_info_count) {
> + WRITE_ONCE(mrioc->intr_info[midx].op_reply_q, NULL);
> + synchronize_irq(pci_irq_vector(mrioc->pdev, midx));
> + }
>
> if (mrioc->enable_segqueue) {
[Severity: High]
Could clearing op_reply_q to NULL here cause a NULL pointer dereference
in polling paths?
Polling paths read the op_reply_q pointer from the intr_info array and pass
it to mpi3mr_process_op_reply_q() without a NULL check:
mpi3mr_op_request_post()
mpi3mr_process_op_reply_q(mrioc, mrioc->intr_info[midx].op_reply_q);
mpi3mr_poll_pend_io_completions()
mpi3mr_process_op_reply_q(mrioc, mrioc->intr_info[i].op_reply_q);
If a concurrent controller reset frees the queues and sets this pointer
to NULL, mpi3mr_process_op_reply_q() will immediately dereference it.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260805110634.346670-1-ranjan.kumar@broadcom.com?part=6
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v4 07/10] mpi3mr: Fix firmware event reference leak during cleanup
2026-08-05 11:06 [PATCH v4 00/10] mpi3mr: Few Enhancements and minor fixes Ranjan Kumar
` (5 preceding siblings ...)
2026-08-05 11:06 ` [PATCH v4 06/10] mpi3mr: Fix memory leak on operational queue creation failure Ranjan Kumar
@ 2026-08-05 11:06 ` Ranjan Kumar
2026-08-05 11:38 ` sashiko-bot
2026-08-05 11:06 ` [PATCH v4 08/10] mpi3mr: Fix SAS port allocation and registration error handling Ranjan Kumar
` (2 subsequent siblings)
9 siblings, 1 reply; 18+ messages in thread
From: Ranjan Kumar @ 2026-08-05 11:06 UTC (permalink / raw)
To: linux-scsi, martin.petersen
Cc: sathya.prakash, chandrakanth.patil, vishakhavc, ipylypiv,
Ranjan Kumar, Sashiko
During firmware event cleanup, when an event is currently executing or
pending at the SCSI mid-layer, the driver sets a discard flag and exits
the cleanup routine early. This early exit skips the normal cancel path,
resulting in the firmware event reference count not being decremented,
leading to a reference leak.
Additionally, resolve the following concurrency issues in the firmware
event handling paths:
1. mpi3mr_cleanup_fwevt_list() read current_event locklessly. It is
now acquired under fwevt_lock.
2. mpi3mr_dequeue_fwevt() dropped the reference before returning it,
risking a use-after-free. The drop is now moved into
mpi3mr_cancel_work().
3. mpi3mr_fwevt_bh() dropped fwevt_lock mid-move, racing with unload.
The move is now inlined under one continuous lock hold.
4. pending_at_sml was read/written without a lock, risking an ABBA
deadlock. It is now protected by fwevt_lock throughout.
5. mpi3mr_suspend() could unmap PCI resources before the event
worker finished. It now flushes the workqueue first.
6. mpi3mr_report_tgtdev_to_host() and mpi3mr_remove_tgtdev_from_host()
could still set pending_at_sml and block in the SCSI mid-layer
after a stop or reset had already begun, deadlocking against the
thread waiting on that flag. Both now bail out beforehand once
stop_drv_processing or reset_in_progress is set.
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/patchset/20260626114109.43685-1-ranjan.kumar@broadcom.com?part=7
Closes: https://sashiko.dev/#/patchset/20260708183305.244485-1-ranjan.kumar@broadcom.com?part=7
Closes: https://sashiko.dev/#/patchset/20260724102505.115136-1-ranjan.kumar@broadcom.com?part=7
Signed-off-by: Chandrakanth Patil <chandrakanth.patil@broadcom.com>
Signed-off-by: Ranjan Kumar <ranjan.kumar@broadcom.com>
---
drivers/scsi/mpi3mr/mpi3mr_os.c | 139 +++++++++++++++++--------
drivers/scsi/mpi3mr/mpi3mr_transport.c | 20 +++-
2 files changed, 109 insertions(+), 50 deletions(-)
diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
index 23a6a5e3df5f..a34ea7e05690 100644
--- a/drivers/scsi/mpi3mr/mpi3mr_os.c
+++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
@@ -282,32 +282,6 @@ void mpi3mr_hdb_trigger_data_event(struct mpi3mr_ioc *mrioc,
mpi3mr_fwevt_add_to_list(mrioc, fwevt);
}
-/**
- * mpi3mr_fwevt_del_from_list - Delete firmware event from list
- * @mrioc: Adapter instance reference
- * @fwevt: Firmware event reference
- *
- * Delete the given firmware event from the firmware event list.
- *
- * Return: Nothing.
- */
-static void mpi3mr_fwevt_del_from_list(struct mpi3mr_ioc *mrioc,
- struct mpi3mr_fwevt *fwevt)
-{
- unsigned long flags;
-
- spin_lock_irqsave(&mrioc->fwevt_lock, flags);
- if (!list_empty(&fwevt->list)) {
- list_del_init(&fwevt->list);
- /*
- * Put fwevt reference count after
- * removing it from fwevt_list
- */
- mpi3mr_fwevt_put(fwevt);
- }
- spin_unlock_irqrestore(&mrioc->fwevt_lock, flags);
-}
-
/**
* mpi3mr_dequeue_fwevt - Dequeue firmware event from the list
* @mrioc: Adapter instance reference
@@ -327,11 +301,7 @@ static struct mpi3mr_fwevt *mpi3mr_dequeue_fwevt(
fwevt = list_first_entry(&mrioc->fwevt_list,
struct mpi3mr_fwevt, list);
list_del_init(&fwevt->list);
- /*
- * Put fwevt reference count after
- * removing it from fwevt_list
- */
- mpi3mr_fwevt_put(fwevt);
+
}
spin_unlock_irqrestore(&mrioc->fwevt_lock, flags);
@@ -365,6 +335,11 @@ static void mpi3mr_cancel_work(struct mpi3mr_fwevt *fwevt)
*/
mpi3mr_fwevt_put(fwevt);
}
+
+ /*
+ * Drop the reference count that was acquired by the caller.
+ */
+ mpi3mr_fwevt_put(fwevt);
}
/**
@@ -379,17 +354,44 @@ static void mpi3mr_cancel_work(struct mpi3mr_fwevt *fwevt)
void mpi3mr_cleanup_fwevt_list(struct mpi3mr_ioc *mrioc)
{
struct mpi3mr_fwevt *fwevt = NULL;
+ unsigned long flags;
+ /*
+ * Safely read current_event under lock to prevent TOCTOU race
+ * with the firmware event worker thread.
+ */
+ spin_lock_irqsave(&mrioc->fwevt_lock, flags);
if ((list_empty(&mrioc->fwevt_list) && !mrioc->current_event) ||
- !mrioc->fwevt_worker_thread)
+ !mrioc->fwevt_worker_thread) {
+ spin_unlock_irqrestore(&mrioc->fwevt_lock, flags);
return;
+ }
+ spin_unlock_irqrestore(&mrioc->fwevt_lock, flags);
while ((fwevt = mpi3mr_dequeue_fwevt(mrioc)))
mpi3mr_cancel_work(fwevt);
- if (mrioc->current_event) {
- fwevt = mrioc->current_event;
+ /*
+ * Safely read current_event under lock to prevent TOCTOU race
+ * with the firmware event worker thread.
+ */
+ spin_lock_irqsave(&mrioc->fwevt_lock, flags);
+ fwevt = mrioc->current_event;
+ if (fwevt) {
+ /*
+ * Take a reference to ensure the event is not freed by the
+ * worker thread while we are evaluating or cancelling it.
+ */
+ mpi3mr_fwevt_get(fwevt);
+ }
+ spin_unlock_irqrestore(&mrioc->fwevt_lock, flags);
+
+ if (fwevt) {
+ bool pending_at_sml;
+
/*
+ * Read pending_at_sml under lock to avoid a stale value.
+ *
* Don't call cancel_work_sync() API for the
* fwevt work if the controller reset is
* get called as part of processing the
@@ -397,8 +399,13 @@ void mpi3mr_cleanup_fwevt_list(struct mpi3mr_ioc *mrioc)
* waiting for device add/remove APIs to complete.
* Otherwise we will see deadlock.
*/
- if (current_work() == &fwevt->work || fwevt->pending_at_sml) {
+ spin_lock_irqsave(&mrioc->fwevt_lock, flags);
+ pending_at_sml = fwevt->pending_at_sml;
+ spin_unlock_irqrestore(&mrioc->fwevt_lock, flags);
+
+ if (current_work() == &fwevt->work || pending_at_sml) {
fwevt->discard = 1;
+ mpi3mr_fwevt_put(fwevt);
return;
}
@@ -912,6 +919,8 @@ void mpi3mr_remove_tgtdev_from_host(struct mpi3mr_ioc *mrioc,
struct mpi3mr_tgt_dev *tgtdev)
{
struct mpi3mr_stgt_priv_data *tgt_priv;
+ unsigned long flags;
+ bool discard = false;
ioc_info(mrioc, "%s :Removing handle(0x%04x), wwid(0x%016llx)\n",
__func__, tgtdev->dev_handle, (unsigned long long)tgtdev->wwid);
@@ -924,17 +933,27 @@ void mpi3mr_remove_tgtdev_from_host(struct mpi3mr_ioc *mrioc,
if (!mrioc->sas_transport_enabled || (tgtdev->dev_type !=
MPI3_DEVICE_DEVFORM_SAS_SATA) || tgtdev->non_stl) {
if (tgtdev->starget) {
+ spin_lock_irqsave(&mrioc->fwevt_lock, flags);
+ if (mrioc->stop_drv_processing ||
+ mrioc->reset_in_progress) {
+ spin_unlock_irqrestore(&mrioc->fwevt_lock, flags);
+ return;
+ }
if (mrioc->current_event)
mrioc->current_event->pending_at_sml = 1;
+ spin_unlock_irqrestore(&mrioc->fwevt_lock, flags);
scsi_remove_target(&tgtdev->starget->dev);
tgtdev->host_exposed = 0;
+ spin_lock_irqsave(&mrioc->fwevt_lock, flags);
if (mrioc->current_event) {
mrioc->current_event->pending_at_sml = 0;
- if (mrioc->current_event->discard) {
- mpi3mr_print_device_event_notice(mrioc,
- false);
- return;
- }
+ discard = mrioc->current_event->discard;
+ }
+ spin_unlock_irqrestore(&mrioc->fwevt_lock, flags);
+ if (discard) {
+ mpi3mr_print_device_event_notice(mrioc,
+ false);
+ return;
}
}
} else
@@ -962,6 +981,8 @@ static int mpi3mr_report_tgtdev_to_host(struct mpi3mr_ioc *mrioc,
{
int retval = 0;
struct mpi3mr_tgt_dev *tgtdev;
+ unsigned long flags;
+ bool discard = false;
if (mrioc->reset_in_progress || mrioc->pci_err_recovery)
return -1;
@@ -978,19 +999,29 @@ static int mpi3mr_report_tgtdev_to_host(struct mpi3mr_ioc *mrioc,
if (!mrioc->sas_transport_enabled || (tgtdev->dev_type !=
MPI3_DEVICE_DEVFORM_SAS_SATA) || tgtdev->non_stl){
tgtdev->host_exposed = 1;
+ spin_lock_irqsave(&mrioc->fwevt_lock, flags);
+ if (mrioc->stop_drv_processing || mrioc->reset_in_progress) {
+ spin_unlock_irqrestore(&mrioc->fwevt_lock, flags);
+ tgtdev->host_exposed = 0;
+ goto out;
+ }
if (mrioc->current_event)
mrioc->current_event->pending_at_sml = 1;
+ spin_unlock_irqrestore(&mrioc->fwevt_lock, flags);
scsi_scan_target(&mrioc->shost->shost_gendev,
mrioc->scsi_device_channel, tgtdev->perst_id,
SCAN_WILD_CARD, SCSI_SCAN_INITIAL);
if (!tgtdev->starget)
tgtdev->host_exposed = 0;
+ spin_lock_irqsave(&mrioc->fwevt_lock, flags);
if (mrioc->current_event) {
mrioc->current_event->pending_at_sml = 0;
- if (mrioc->current_event->discard) {
- mpi3mr_print_device_event_notice(mrioc, true);
- goto out;
- }
+ discard = mrioc->current_event->discard;
+ }
+ spin_unlock_irqrestore(&mrioc->fwevt_lock, flags);
+ if (discard) {
+ mpi3mr_print_device_event_notice(mrioc, true);
+ goto out;
}
dprint_event_bh(mrioc,
"exposed target device with handle(0x%04x), perst_id(%d)\n",
@@ -2133,9 +2164,19 @@ static void mpi3mr_fwevt_bh(struct mpi3mr_ioc *mrioc,
u16 perst_id, handle, dev_info;
struct mpi3_device0_sas_sata_format *sasinf = NULL;
unsigned int timeout;
+ unsigned long flags;
- mpi3mr_fwevt_del_from_list(mrioc, fwevt);
+ spin_lock_irqsave(&mrioc->fwevt_lock, flags);
+ if (!list_empty(&fwevt->list)) {
+ list_del_init(&fwevt->list);
+ /*
+ * Put fwevt reference count after
+ * removing it from fwevt_list
+ */
+ mpi3mr_fwevt_put(fwevt);
+ }
mrioc->current_event = fwevt;
+ spin_unlock_irqrestore(&mrioc->fwevt_lock, flags);
if (mrioc->stop_drv_processing) {
dprint_event_bh(mrioc, "ignoring event(0x%02x) in the bottom half handler\n"
@@ -2268,9 +2309,12 @@ static void mpi3mr_fwevt_bh(struct mpi3mr_ioc *mrioc,
mpi3mr_process_event_ack(mrioc, fwevt->event_id,
fwevt->evt_ctx);
out:
+ spin_lock_irqsave(&mrioc->fwevt_lock, flags);
+ mrioc->current_event = NULL;
+ spin_unlock_irqrestore(&mrioc->fwevt_lock, flags);
+
/* Put fwevt reference count to neutralize kref_init increment */
mpi3mr_fwevt_put(fwevt);
- mrioc->current_event = NULL;
}
/**
@@ -5811,6 +5855,9 @@ mpi3mr_suspend(struct device *dev)
ssleep(1);
mrioc->stop_drv_processing = 1;
mpi3mr_cleanup_fwevt_list(mrioc);
+ /* Flush any pending discarded event before unmapping PCI resources below. */
+ if (mrioc->fwevt_worker_thread)
+ flush_workqueue(mrioc->fwevt_worker_thread);
scsi_block_requests(shost);
mpi3mr_stop_watchdog(mrioc);
mpi3mr_cleanup_ioc(mrioc);
diff --git a/drivers/scsi/mpi3mr/mpi3mr_transport.c b/drivers/scsi/mpi3mr/mpi3mr_transport.c
index 240f67a8e2e3..b309cfdf6687 100644
--- a/drivers/scsi/mpi3mr/mpi3mr_transport.c
+++ b/drivers/scsi/mpi3mr/mpi3mr_transport.c
@@ -1330,6 +1330,7 @@ static struct mpi3mr_sas_port *mpi3mr_sas_port_add(struct mpi3mr_ioc *mrioc,
struct mpi3mr_sas_phy *mr_sas_phy, *next;
struct mpi3mr_sas_port *mr_sas_port;
unsigned long flags;
+ bool discard = false;
struct mpi3mr_sas_node *mr_sas_node;
struct sas_rphy *rphy;
struct mpi3mr_tgt_dev *tgtdev = NULL;
@@ -1457,8 +1458,10 @@ static struct mpi3mr_sas_port *mpi3mr_sas_port_add(struct mpi3mr_ioc *mrioc,
}
rphy->identify = mr_sas_port->remote_identify;
+ spin_lock_irqsave(&mrioc->fwevt_lock, flags);
if (mrioc->current_event)
mrioc->current_event->pending_at_sml = 1;
+ spin_unlock_irqrestore(&mrioc->fwevt_lock, flags);
if ((sas_rphy_add(rphy))) {
ioc_err(mrioc, "failure at %s:%d/%s()!\n",
@@ -1480,11 +1483,14 @@ static struct mpi3mr_sas_port *mpi3mr_sas_port_add(struct mpi3mr_ioc *mrioc,
list_add_tail(&mr_sas_port->port_list, &mr_sas_node->sas_port_list);
spin_unlock_irqrestore(&mrioc->sas_node_lock, flags);
+ spin_lock_irqsave(&mrioc->fwevt_lock, flags);
if (mrioc->current_event) {
mrioc->current_event->pending_at_sml = 0;
- if (mrioc->current_event->discard)
- mpi3mr_print_device_event_notice(mrioc, true);
+ discard = mrioc->current_event->discard;
}
+ spin_unlock_irqrestore(&mrioc->fwevt_lock, flags);
+ if (discard)
+ mpi3mr_print_device_event_notice(mrioc, true);
/* fill in report manufacture */
if (mr_sas_port->remote_identify.device_type ==
@@ -1522,6 +1528,7 @@ static void mpi3mr_sas_port_remove(struct mpi3mr_ioc *mrioc, u64 sas_address,
{
int i;
unsigned long flags;
+ bool discard = false;
struct mpi3mr_sas_port *mr_sas_port, *next;
struct mpi3mr_sas_node *mr_sas_node;
u8 found = 0;
@@ -1578,8 +1585,10 @@ static void mpi3mr_sas_port_remove(struct mpi3mr_ioc *mrioc, u64 sas_address,
spin_unlock_irqrestore(&mrioc->sas_node_lock, flags);
+ spin_lock_irqsave(&mrioc->fwevt_lock, flags);
if (mrioc->current_event)
mrioc->current_event->pending_at_sml = 1;
+ spin_unlock_irqrestore(&mrioc->fwevt_lock, flags);
list_for_each_entry_safe(mr_sas_phy, next_phy,
&mr_sas_port->phy_list, port_siblings) {
@@ -1601,11 +1610,14 @@ static void mpi3mr_sas_port_remove(struct mpi3mr_ioc *mrioc, u64 sas_address,
ioc_info(mrioc, "%s: removed sas_address(0x%016llx)\n",
__func__, (unsigned long long)sas_address);
+ spin_lock_irqsave(&mrioc->fwevt_lock, flags);
if (mrioc->current_event) {
mrioc->current_event->pending_at_sml = 0;
- if (mrioc->current_event->discard)
- mpi3mr_print_device_event_notice(mrioc, false);
+ discard = mrioc->current_event->discard;
}
+ spin_unlock_irqrestore(&mrioc->fwevt_lock, flags);
+ if (discard)
+ mpi3mr_print_device_event_notice(mrioc, false);
kfree(mr_sas_port);
}
--
2.47.3
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v4 07/10] mpi3mr: Fix firmware event reference leak during cleanup
2026-08-05 11:06 ` [PATCH v4 07/10] mpi3mr: Fix firmware event reference leak during cleanup Ranjan Kumar
@ 2026-08-05 11:38 ` sashiko-bot
0 siblings, 0 replies; 18+ messages in thread
From: sashiko-bot @ 2026-08-05 11:38 UTC (permalink / raw)
To: Ranjan Kumar; +Cc: linux-scsi
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues:
- [High] Race condition / TOCTOU when accessing fwevt_worker_thread can cause a NULL pointer dereference.
- [High] Incomplete deadlock fix in SAS transport path allows ABBA deadlock with SCSI mid-layer.
- [Medium] Memory leak of firmware event when workqueue is not available.
--
--- Patch [7]: [PATCH v4 07/10] mpi3mr: Fix firmware event reference leak during cleanup ---
commit 02873552fdbb7ea8056e6479b78735dc33631a59
Author: Ranjan Kumar <ranjan.kumar@broadcom.com>
mpi3mr: Fix firmware event reference leak during cleanup
This patch fixes a reference leak during firmware event cleanup when an event
is currently executing or pending at the SCSI mid-layer. It also resolves
multiple concurrency and deadlock issues in the firmware event handling paths.
Closes: https://sashiko.dev/#/patchset/20260626114109.43685-1-ranjan.kumar@broadcom.com?part=7
Closes: https://sashiko.dev/#/patchset/20260708183305.244485-1-ranjan.kumar@broadcom.com?part=7
Closes: https://sashiko.dev/#/patchset/20260724102505.115136-1-ranjan.kumar@broadcom.com?part=7
> diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
> index 23a6a5e3df5f4..a34ea7e056900 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_os.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
> @@ -282,32 +282,6 @@ void mpi3mr_hdb_trigger_data_event(struct mpi3mr_ioc *mrioc,
> mpi3mr_fwevt_add_to_list(mrioc, fwevt);
> }
[Severity: High]
This is a pre-existing issue, but can the lockless check in
mpi3mr_fwevt_add_to_list() cause a race condition leading to a NULL pointer
dereference?
If mpi3mr_fwevt_add_to_list() checks mrioc->fwevt_worker_thread locklessly:
if (!mrioc->fwevt_worker_thread)
return;
And concurrently, during driver shutdown or unload, the workqueue pointer is
set to NULL while holding fwevt_lock:
spin_lock_irqsave(&mrioc->fwevt_lock, flags);
wq = mrioc->fwevt_worker_thread;
mrioc->fwevt_worker_thread = NULL;
spin_unlock_irqrestore(&mrioc->fwevt_lock, flags);
Could mpi3mr_fwevt_add_to_list() pass the lockless check, then block on
fwevt_lock, and after acquiring the lock, unconditionally pass the now-NULL
mrioc->fwevt_worker_thread to queue_work()?
[Severity: Medium]
This is also a pre-existing issue, but does the early return in
mpi3mr_fwevt_add_to_list() leak the firmware event?
if (!mrioc->fwevt_worker_thread)
return;
Since mpi3mr_alloc_fwevt() initializes the reference count to 1, if the worker
thread is already gone and we take this early return, do we need to call
mpi3mr_fwevt_put() to prevent leaking the allocated firmware event?
> -/**
> - * mpi3mr_fwevt_del_from_list - Delete firmware event from list
> - * @mrioc: Adapter instance reference
> - * @fwevt: Firmware event reference
[ ... ]
> diff --git a/drivers/scsi/mpi3mr/mpi3mr_transport.c b/drivers/scsi/mpi3mr/mpi3mr_transport.c
> index 240f67a8e2e3b..b309cfdf66873 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_transport.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_transport.c
[ ... ]
> @@ -1457,8 +1458,10 @@ static struct mpi3mr_sas_port *mpi3mr_sas_port_add(struct mpi3mr_ioc *mrioc,
> }
> rphy->identify = mr_sas_port->remote_identify;
>
> + spin_lock_irqsave(&mrioc->fwevt_lock, flags);
> if (mrioc->current_event)
> mrioc->current_event->pending_at_sml = 1;
> + spin_unlock_irqrestore(&mrioc->fwevt_lock, flags);
>
> if ((sas_rphy_add(rphy))) {
> ioc_err(mrioc, "failure at %s:%d/%s()!\n",
[Severity: High]
This is a pre-existing issue, but does this code allow an ABBA deadlock with
the SCSI mid-layer?
Unlike mpi3mr_report_tgtdev_to_host(), this function does not check
mrioc->reset_in_progress or mrioc->stop_drv_processing before setting
pending_at_sml = 1 and calling sas_rphy_add().
If a controller reset initiates, it waits for the event worker via
cancel_work_sync(). If the worker thread is here and calls sas_rphy_add(),
the SML commands might receive SCSI_MLQUEUE_HOST_BUSY from
mpi3mr_queuecommand() because the reset is active.
Since SML will retry infinitely while the reset is underway, does this cause
the event worker thread to deadlock against the thread waiting for the reset
to complete?
[ ... ]
> @@ -1578,8 +1585,10 @@ static void mpi3mr_sas_port_remove(struct mpi3mr_ioc *mrioc, u64 sas_address,
>
> spin_unlock_irqrestore(&mrioc->sas_node_lock, flags);
>
> + spin_lock_irqsave(&mrioc->fwevt_lock, flags);
> if (mrioc->current_event)
> mrioc->current_event->pending_at_sml = 1;
> + spin_unlock_irqrestore(&mrioc->fwevt_lock, flags);
>
> list_for_each_entry_safe(mr_sas_phy, next_phy,
> &mr_sas_port->phy_list, port_siblings) {
[Severity: High]
This is a pre-existing issue, but this appears to be susceptible to the same
deadlock with the SCSI mid-layer.
If a reset is in progress, setting pending_at_sml = 1 without a bailout
check can cause the worker thread to hang if the SML commands retry
infinitely due to the active reset. Should this have the same bailout check
as mpi3mr_remove_tgtdev_from_host()?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260805110634.346670-1-ranjan.kumar@broadcom.com?part=7
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v4 08/10] mpi3mr: Fix SAS port allocation and registration error handling
2026-08-05 11:06 [PATCH v4 00/10] mpi3mr: Few Enhancements and minor fixes Ranjan Kumar
` (6 preceding siblings ...)
2026-08-05 11:06 ` [PATCH v4 07/10] mpi3mr: Fix firmware event reference leak during cleanup Ranjan Kumar
@ 2026-08-05 11:06 ` Ranjan Kumar
2026-08-05 11:40 ` sashiko-bot
2026-08-05 11:06 ` [PATCH v4 09/10] mpi3mr: Fix SAS PHY cleanup in host addition error paths Ranjan Kumar
2026-08-05 11:06 ` [PATCH v4 10/10] mpi3mr: Driver version update to 8.18.0.8.50 Ranjan Kumar
9 siblings, 1 reply; 18+ messages in thread
From: Ranjan Kumar @ 2026-08-05 11:06 UTC (permalink / raw)
To: linux-scsi, martin.petersen
Cc: sathya.prakash, chandrakanth.patil, vishakhavc, ipylypiv,
Ranjan Kumar, Sashiko
During SAS port creation, the driver does not verify successful port
allocation before attempting registration, which can lead to a NULL
pointer dereference. Additionally, if registration fails, the allocated
port is not freed, resulting in a memory leak.
Fix this by adding a NULL check after allocation and freeing the port
when registration fails.
Additional fixes in the error handling path include:
1. Fixing similar missing NULL checks for rphy allocations.
2. Cleaning up after a failed rphy registration tried to remove a
device that was never added, causing a crash. The rphy is now
freed directly instead.
3. A failed rphy registration left the target device with a dangling
pointer and a stuck pending flag. Both are now cleared.
4. Phys removed on error kept an internal flag set, permanently
blocking them from being added to a port again. Now cleared
alongside the list removal.
5. Could block in the SCSI mid-layer after a stop or reset had
already begun, the same ABBA deadlock class fixed elsewhere. Both
paths now stop before that call once that is detected.
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/patchset/20260626114109.43685-1-ranjan.kumar@broadcom.com?part=8
Closes: https://sashiko.dev/#/patchset/20260708183305.244485-1-ranjan.kumar@broadcom.com?part=8
Closes: https://sashiko.dev/#/patchset/20260724102505.115136-1-ranjan.kumar@broadcom.com?part=8
Signed-off-by: Chandrakanth Patil <chandrakanth.patil@broadcom.com>
Signed-off-by: Ranjan Kumar <ranjan.kumar@broadcom.com>
---
drivers/scsi/mpi3mr/mpi3mr_transport.c | 50 ++++++++++++++++++++++++--
1 file changed, 47 insertions(+), 3 deletions(-)
diff --git a/drivers/scsi/mpi3mr/mpi3mr_transport.c b/drivers/scsi/mpi3mr/mpi3mr_transport.c
index b309cfdf6687..db9cb0b03b9f 100644
--- a/drivers/scsi/mpi3mr/mpi3mr_transport.c
+++ b/drivers/scsi/mpi3mr/mpi3mr_transport.c
@@ -1429,9 +1429,15 @@ static struct mpi3mr_sas_port *mpi3mr_sas_port_add(struct mpi3mr_ioc *mrioc,
}
port = sas_port_alloc_num(mr_sas_node->parent_dev);
+ if (!port) {
+ ioc_err(mrioc, "failure at %s:%d/%s() (sas_port_alloc)!\n",
+ __FILE__, __LINE__, __func__);
+ goto out_fail;
+ }
if ((sas_port_add(port))) {
ioc_err(mrioc, "failure at %s:%d/%s()!\n",
__FILE__, __LINE__, __func__);
+ sas_port_free(port);
goto out_fail;
}
@@ -1451,14 +1457,32 @@ static struct mpi3mr_sas_port *mpi3mr_sas_port_add(struct mpi3mr_ioc *mrioc,
mr_sas_port->port = port;
if (mr_sas_port->remote_identify.device_type == SAS_END_DEVICE) {
rphy = sas_end_device_alloc(port);
+ if (!rphy) {
+ ioc_err(mrioc, "failure at %s:%d/%s() (sas_end_device_alloc)!\n",
+ __FILE__, __LINE__, __func__);
+ sas_port_delete(port);
+ goto out_fail;
+ }
tgtdev->dev_spec.sas_sata_inf.rphy = rphy;
} else {
rphy = sas_expander_alloc(port,
mr_sas_port->remote_identify.device_type);
+ if (!rphy) {
+ ioc_err(mrioc, "failure at %s:%d/%s() (sas_expander_alloc)!\n",
+ __FILE__, __LINE__, __func__);
+ sas_port_delete(port);
+ goto out_fail;
+ }
}
rphy->identify = mr_sas_port->remote_identify;
spin_lock_irqsave(&mrioc->fwevt_lock, flags);
+ if (mrioc->stop_drv_processing || mrioc->reset_in_progress) {
+ spin_unlock_irqrestore(&mrioc->fwevt_lock, flags);
+ sas_rphy_free(rphy);
+ sas_port_delete(port);
+ goto out_fail;
+ }
if (mrioc->current_event)
mrioc->current_event->pending_at_sml = 1;
spin_unlock_irqrestore(&mrioc->fwevt_lock, flags);
@@ -1466,6 +1490,18 @@ static struct mpi3mr_sas_port *mpi3mr_sas_port_add(struct mpi3mr_ioc *mrioc,
if ((sas_rphy_add(rphy))) {
ioc_err(mrioc, "failure at %s:%d/%s()!\n",
__FILE__, __LINE__, __func__);
+ spin_lock_irqsave(&mrioc->fwevt_lock, flags);
+ if (mrioc->current_event) {
+ mrioc->current_event->pending_at_sml = 0;
+ discard = mrioc->current_event->discard;
+ }
+ spin_unlock_irqrestore(&mrioc->fwevt_lock, flags);
+ if (discard)
+ mpi3mr_print_device_event_notice(mrioc, true);
+ sas_rphy_unlink(rphy);
+ sas_rphy_free(rphy);
+ sas_port_delete(port);
+ goto out_fail;
}
if (mr_sas_port->remote_identify.device_type == SAS_END_DEVICE) {
tgtdev->dev_spec.sas_sata_inf.pend_sas_rphy_add = 0;
@@ -1504,9 +1540,17 @@ static struct mpi3mr_sas_port *mpi3mr_sas_port_add(struct mpi3mr_ioc *mrioc,
return mr_sas_port;
out_fail:
+ if (tgtdev) {
+ tgtdev->dev_spec.sas_sata_inf.pend_sas_rphy_add = 0;
+ tgtdev->dev_spec.sas_sata_inf.rphy = NULL;
+ mpi3mr_tgtdev_put(tgtdev);
+ }
+
list_for_each_entry_safe(mr_sas_phy, next, &mr_sas_port->phy_list,
- port_siblings)
+ port_siblings) {
+ mr_sas_phy->phy_belongs_to_port = 0;
list_del(&mr_sas_phy->port_siblings);
+ }
kfree(mr_sas_port);
return NULL;
}
@@ -1600,12 +1644,12 @@ static void mpi3mr_sas_port_remove(struct mpi3mr_ioc *mrioc, u64 sas_address,
mr_sas_port->remote_identify.sas_address,
mr_sas_phy->phy_id);
mr_sas_phy->phy_belongs_to_port = 0;
- if (!mrioc->stop_drv_processing)
+ if (!mrioc->stop_drv_processing && !mrioc->reset_in_progress)
sas_port_delete_phy(mr_sas_port->port,
mr_sas_phy->phy);
list_del(&mr_sas_phy->port_siblings);
}
- if (!mrioc->stop_drv_processing)
+ if (!mrioc->stop_drv_processing && !mrioc->reset_in_progress)
sas_port_delete(mr_sas_port->port);
ioc_info(mrioc, "%s: removed sas_address(0x%016llx)\n",
__func__, (unsigned long long)sas_address);
--
2.47.3
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v4 08/10] mpi3mr: Fix SAS port allocation and registration error handling
2026-08-05 11:06 ` [PATCH v4 08/10] mpi3mr: Fix SAS port allocation and registration error handling Ranjan Kumar
@ 2026-08-05 11:40 ` sashiko-bot
0 siblings, 0 replies; 18+ messages in thread
From: sashiko-bot @ 2026-08-05 11:40 UTC (permalink / raw)
To: Ranjan Kumar; +Cc: linux-scsi
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Skipping `sas_port_delete` and `sas_port_delete_phy` during a controller reset causes permanent memory leaks of SAS transport objects and triggers a kernel `BUG()` on subsequent device discovery.
--
--- Patch [8]: [PATCH v4 08/10] mpi3mr: Fix SAS port allocation and registration error handling ---
commit 2a61369a5f802bef404aa18e7c7a2583f37c4fc7
Author: Ranjan Kumar <ranjan.kumar@broadcom.com>
mpi3mr: Fix SAS port allocation and registration error handling
This commit adds NULL checks after SAS port and rphy allocations to prevent
NULL pointer dereferences. It cleans up memory and state if registration
fails, resolving memory leaks and dangling pointers. Finally, it prevents
deadlocks in the SCSI mid-layer by checking for resets or driver stops.
> diff --git a/drivers/scsi/mpi3mr/mpi3mr_transport.c b/drivers/scsi/mpi3mr/mpi3mr_transport.c
> index b309cfdf66873..db9cb0b03b9ff 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_transport.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_transport.c
[ ... ]
> @@ -1600,12 +1644,12 @@ static void mpi3mr_sas_port_remove(struct mpi3mr_ioc *mrioc, u64 sas_address,
> mr_sas_port->remote_identify.sas_address,
> mr_sas_phy->phy_id);
> mr_sas_phy->phy_belongs_to_port = 0;
> - if (!mrioc->stop_drv_processing)
> + if (!mrioc->stop_drv_processing && !mrioc->reset_in_progress)
> sas_port_delete_phy(mr_sas_port->port,
> mr_sas_phy->phy);
> list_del(&mr_sas_phy->port_siblings);
> }
> - if (!mrioc->stop_drv_processing)
> + if (!mrioc->stop_drv_processing && !mrioc->reset_in_progress)
> sas_port_delete(mr_sas_port->port);
[Severity: Critical]
Does skipping sas_port_delete() and sas_port_delete_phy() during a reset cause
a memory leak and a kernel BUG() during rediscovery?
Because mr_sas_port is unconditionally freed at the end of
mpi3mr_sas_port_remove(), the transport objects (sas_port and sas_rphy) are
leaked when their delete functions are bypassed.
Additionally, the sas_phy remains linked to the leaked port's phy_list. When
the reset completes and the device comes back online, mpi3mr_sas_port_add()
will create a new port and call sas_port_add_phy() on the same sas_phy.
The SAS transport layer detects the phy is already linked to another port and
intentionally triggers a BUG().
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260805110634.346670-1-ranjan.kumar@broadcom.com?part=8
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v4 09/10] mpi3mr: Fix SAS PHY cleanup in host addition error paths
2026-08-05 11:06 [PATCH v4 00/10] mpi3mr: Few Enhancements and minor fixes Ranjan Kumar
` (7 preceding siblings ...)
2026-08-05 11:06 ` [PATCH v4 08/10] mpi3mr: Fix SAS port allocation and registration error handling Ranjan Kumar
@ 2026-08-05 11:06 ` Ranjan Kumar
2026-08-05 11:06 ` [PATCH v4 10/10] mpi3mr: Driver version update to 8.18.0.8.50 Ranjan Kumar
9 siblings, 0 replies; 18+ messages in thread
From: Ranjan Kumar @ 2026-08-05 11:06 UTC (permalink / raw)
To: linux-scsi, martin.petersen
Cc: sathya.prakash, chandrakanth.patil, vishakhavc, ipylypiv,
Ranjan Kumar, Sashiko
When adding a SAS host, the driver allocates a PHY array and subsequently
creates individual SAS PHYs. If a later step fails, the error path exits
without cleaning up previously allocated resources, resulting in leaks of
both the PHY array and any registered SAS PHYs.
Additionally, the return value of mpi3mr_add_host_phy() was being ignored.
If it failed, mr_sas_phy->phy would be left as NULL, which could later
lead to a NULL pointer dereference in mpi3mr_sas_port_add() when the
attached device triggers a device addition event.
Add a dedicated cleanup path that deletes any successfully created SAS
PHYs and frees the PHY array before returning from initialization
failure paths. Also, check the return value of mpi3mr_add_host_phy()
and jump to the cleanup path on failure.
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/patchset/20260626114109.43685-1-ranjan.kumar@broadcom.com?part=9
Signed-off-by: Chandrakanth Patil <chandrakanth.patil@broadcom.com>
Signed-off-by: Ranjan Kumar <ranjan.kumar@broadcom.com>
---
drivers/scsi/mpi3mr/mpi3mr_transport.c | 33 ++++++++++++++++++--------
1 file changed, 23 insertions(+), 10 deletions(-)
diff --git a/drivers/scsi/mpi3mr/mpi3mr_transport.c b/drivers/scsi/mpi3mr/mpi3mr_transport.c
index db9cb0b03b9f..e4c223a0acf6 100644
--- a/drivers/scsi/mpi3mr/mpi3mr_transport.c
+++ b/drivers/scsi/mpi3mr/mpi3mr_transport.c
@@ -1216,13 +1216,14 @@ void mpi3mr_sas_host_add(struct mpi3mr_ioc *mrioc)
}
num_phys = sas_io_unit_pg0->num_phys;
kfree(sas_io_unit_pg0);
+ sas_io_unit_pg0 = NULL;
mrioc->sas_hba.host_node = 1;
INIT_LIST_HEAD(&mrioc->sas_hba.sas_port_list);
mrioc->sas_hba.parent_dev = &mrioc->shost->shost_gendev;
mrioc->sas_hba.phy = kzalloc_objs(struct mpi3mr_sas_phy, num_phys);
if (!mrioc->sas_hba.phy)
- return;
+ goto out;
mrioc->sas_hba.num_phys = num_phys;
@@ -1230,12 +1231,12 @@ void mpi3mr_sas_host_add(struct mpi3mr_ioc *mrioc)
(num_phys * sizeof(struct mpi3_sas_io_unit0_phy_data));
sas_io_unit_pg0 = kzalloc(sz, GFP_KERNEL);
if (!sas_io_unit_pg0)
- return;
+ goto out_free_phy;
if (mpi3mr_cfg_get_sas_io_unit_pg0(mrioc, sas_io_unit_pg0, sz)) {
ioc_err(mrioc, "failure at %s:%d/%s()!\n",
__FILE__, __LINE__, __func__);
- goto out;
+ goto out_free_phy;
}
mrioc->sas_hba.handle = 0;
@@ -1249,12 +1250,12 @@ void mpi3mr_sas_host_add(struct mpi3mr_ioc *mrioc)
MPI3_SAS_PHY_PGAD_FORM_PHY_NUMBER, i)) {
ioc_err(mrioc, "failure at %s:%d/%s()!\n",
__FILE__, __LINE__, __func__);
- goto out;
+ goto out_free_phy;
}
if (ioc_status != MPI3_IOCSTATUS_SUCCESS) {
ioc_err(mrioc, "failure at %s:%d/%s()!\n",
__FILE__, __LINE__, __func__);
- goto out;
+ goto out_free_phy;
}
if (!mrioc->sas_hba.handle)
@@ -1264,26 +1265,27 @@ void mpi3mr_sas_host_add(struct mpi3mr_ioc *mrioc)
if (!(mpi3mr_get_hba_port_by_id(mrioc, port_id)))
if (!mpi3mr_alloc_hba_port(mrioc, port_id))
- goto out;
+ goto out_free_phy;
mrioc->sas_hba.phy[i].handle = mrioc->sas_hba.handle;
mrioc->sas_hba.phy[i].phy_id = i;
mrioc->sas_hba.phy[i].hba_port =
mpi3mr_get_hba_port_by_id(mrioc, port_id);
- mpi3mr_add_host_phy(mrioc, &mrioc->sas_hba.phy[i],
- phy_pg0, mrioc->sas_hba.parent_dev);
+ if (mpi3mr_add_host_phy(mrioc, &mrioc->sas_hba.phy[i],
+ phy_pg0, mrioc->sas_hba.parent_dev))
+ goto out_free_phy;
}
if ((mpi3mr_cfg_get_dev_pg0(mrioc, &ioc_status, &dev_pg0,
sizeof(dev_pg0), MPI3_DEVICE_PGAD_FORM_HANDLE,
mrioc->sas_hba.handle))) {
ioc_err(mrioc, "%s: device page0 read failed\n", __func__);
- goto out;
+ goto out_free_phy;
}
if (ioc_status != MPI3_IOCSTATUS_SUCCESS) {
ioc_err(mrioc, "device page read failed for handle(0x%04x), with ioc_status(0x%04x) failure at %s:%d/%s()!\n",
mrioc->sas_hba.handle, ioc_status, __FILE__, __LINE__,
__func__);
- goto out;
+ goto out_free_phy;
}
mrioc->sas_hba.enclosure_handle =
le16_to_cpu(dev_pg0.enclosure_handle);
@@ -1306,6 +1308,17 @@ void mpi3mr_sas_host_add(struct mpi3mr_ioc *mrioc)
le64_to_cpu(encl_pg0.enclosure_logical_id);
}
+ goto out;
+
+out_free_phy:
+ for (i = 0; i < mrioc->sas_hba.num_phys; i++) {
+ if (mrioc->sas_hba.phy[i].phy)
+ sas_phy_delete(mrioc->sas_hba.phy[i].phy);
+ }
+ kfree(mrioc->sas_hba.phy);
+ mrioc->sas_hba.phy = NULL;
+ mrioc->sas_hba.num_phys = 0;
+
out:
kfree(sas_io_unit_pg0);
}
--
2.47.3
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH v4 10/10] mpi3mr: Driver version update to 8.18.0.8.50
2026-08-05 11:06 [PATCH v4 00/10] mpi3mr: Few Enhancements and minor fixes Ranjan Kumar
` (8 preceding siblings ...)
2026-08-05 11:06 ` [PATCH v4 09/10] mpi3mr: Fix SAS PHY cleanup in host addition error paths Ranjan Kumar
@ 2026-08-05 11:06 ` Ranjan Kumar
9 siblings, 0 replies; 18+ messages in thread
From: Ranjan Kumar @ 2026-08-05 11:06 UTC (permalink / raw)
To: linux-scsi, martin.petersen
Cc: sathya.prakash, chandrakanth.patil, vishakhavc, ipylypiv,
Ranjan Kumar
Update driver version to 8.18.0.8.50
Signed-off-by: Ranjan Kumar <ranjan.kumar@broadcom.com>
---
drivers/scsi/mpi3mr/mpi3mr.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/mpi3mr/mpi3mr.h b/drivers/scsi/mpi3mr/mpi3mr.h
index 4d19a9460d38..d6e16707fd97 100644
--- a/drivers/scsi/mpi3mr/mpi3mr.h
+++ b/drivers/scsi/mpi3mr/mpi3mr.h
@@ -56,8 +56,8 @@ extern struct list_head mrioc_list;
extern int prot_mask;
extern atomic64_t event_counter;
-#define MPI3MR_DRIVER_VERSION "8.17.0.3.50"
-#define MPI3MR_DRIVER_RELDATE "09-January-2026"
+#define MPI3MR_DRIVER_VERSION "8.18.0.8.50"
+#define MPI3MR_DRIVER_RELDATE "26-June-2026"
#define MPI3MR_DRIVER_NAME "mpi3mr"
#define MPI3MR_DRIVER_LICENSE "GPL"
--
2.47.3
^ permalink raw reply related [flat|nested] 18+ messages in thread