Skip to contentSkip to conversation

This patchset contains critical Bug fixes

PATCH Applied Ranjan Kumar 3 participants 10 messages Feb 28, 2023 – Apr 3, 2023

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

cover This patchset contains critical Bug fixes

This patchset contains critical Bug fixes

Ranjan Kumar (6):
mpi3mr: IOCTL timeout when disable/enable Interpt
mpi3mr: Driver unload crash host when enhanced logging is enabled
mpi3mr: Wait for diagnostic save during controller init
mpi3mr: appropriate return values for failures in firmware init path
mpi3mr: NVMe commands size greater than 8K fails
mpi3mr: Bad drive in topology results kernel crash

 drivers/scsi/mpi3mr/mpi3mr.h           |  3 ++
 drivers/scsi/mpi3mr/mpi3mr_app.c       |  4 +--
 drivers/scsi/mpi3mr/mpi3mr_fw.c        | 42 +++++++++++++++++++++++---
 drivers/scsi/mpi3mr/mpi3mr_os.c        |  1 +
 drivers/scsi/mpi3mr/mpi3mr_transport.c | 15 +++++----
 5 files changed, 52 insertions(+), 13 deletions(-)
Signature · 1 line
2.31.1
#1 lore raw

PATCH 1/6 mpi3mr: IOCTL timeout when disable/enable Interpt

As part of Task Management handling, the driver will disable and enable the MSIx index zero which belongs to the Admin reply queue. And while enabling the interrupts driver loses some interrupts and it leads to Admin requests such as IOCTL timeout. So, after enabling the interrupts, poll the Admin reply queue to avoid timeouts.

---
 drivers/scsi/mpi3mr/mpi3mr.h    |  3 +++
 drivers/scsi/mpi3mr/mpi3mr_fw.c | 12 ++++++++++--
 drivers/scsi/mpi3mr/mpi3mr_os.c |  1 +
 3 files changed, 14 insertions(+), 2 deletions(-)
drivers/scsi/mpi3mr/mpi3mr.h +3 -0
index def4c5e15cd8..24e6b1f71107 100644
@@ -903,6 +903,7 @@ struct scmd_priv {
  * @admin_reply_ephase:Admin reply queue expected phase
  * @admin_reply_base: Admin reply queue base virtual address
  * @admin_reply_dma: Admin reply queue base dma address
+ * @admin_reply_q_in_use: Queue is handled by poll/ISR
  * @ready_timeout: Controller ready timeout
  * @intr_info: Interrupt cookie pointer
  * @intr_info_count: Number of interrupt cookies
@@ -1059,6 +1060,7 @@ struct mpi3mr_ioc {
 	u8 admin_reply_ephase;
 	void *admin_reply_base;
 	dma_addr_t admin_reply_dma;
+	atomic_t admin_reply_q_in_use;
 
 	u32 ready_timeout;
 
@@ -1397,4 +1399,5 @@ void mpi3mr_add_event_wait_for_device_refresh(struct mpi3mr_ioc *mrioc);
 void mpi3mr_flush_drv_cmds(struct mpi3mr_ioc *mrioc);
 void mpi3mr_flush_cmds_for_unrecovered_controller(struct mpi3mr_ioc *mrioc);
 void mpi3mr_free_enclosure_list(struct mpi3mr_ioc *mrioc);
+int mpi3mr_process_admin_reply_q(struct mpi3mr_ioc *mrioc);
 #endif /*MPI3MR_H_INCLUDED*/
drivers/scsi/mpi3mr/mpi3mr_fw.c +10 -2
index 0c4aabaefdcc..e7ea3f7b9205 100644
@@ -415,7 +415,7 @@ static void mpi3mr_process_admin_reply_desc(struct mpi3mr_ioc *mrioc,
 		    le64_to_cpu(scsi_reply->sense_data_buffer_address));
 }
 
-static int mpi3mr_process_admin_reply_q(struct mpi3mr_ioc *mrioc)
+int mpi3mr_process_admin_reply_q(struct mpi3mr_ioc *mrioc)
 {
 	u32 exp_phase = mrioc->admin_reply_ephase;
 	u32 admin_reply_ci = mrioc->admin_reply_ci;
@@ -423,12 +423,17 @@ static int mpi3mr_process_admin_reply_q(struct mpi3mr_ioc *mrioc)
 	u64 reply_dma = 0;
 	struct mpi3_default_reply_descriptor *reply_desc;
 
+	if (!atomic_add_unless(&mrioc->admin_reply_q_in_use, 1, 1))
+		return 0;
+
 	reply_desc = (struct mpi3_default_reply_descriptor *)mrioc->admin_reply_base +
 	    admin_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) {
+		atomic_dec(&mrioc->admin_reply_q_in_use);
 		return 0;
+	}
 
 	do {
 		if (mrioc->unrecoverable)
@@ -454,6 +459,7 @@ static int mpi3mr_process_admin_reply_q(struct mpi3mr_ioc *mrioc)
 	writel(admin_reply_ci, &mrioc->sysif_regs->admin_reply_queue_ci);
 	mrioc->admin_reply_ci = admin_reply_ci;
 	mrioc->admin_reply_ephase = exp_phase;
+	atomic_dec(&mrioc->admin_reply_q_in_use);
 
 	return num_admin_replies;
 }
@@ -2608,6 +2614,7 @@ static int mpi3mr_setup_admin_qpair(struct mpi3mr_ioc *mrioc)
 	mrioc->admin_reply_ci = 0;
 	mrioc->admin_reply_ephase = 1;
 	mrioc->admin_reply_base = NULL;
+	atomic_set(&mrioc->admin_reply_q_in_use, 0);
 
 	if (!mrioc->admin_req_base) {
 		mrioc->admin_req_base = dma_alloc_coherent(&mrioc->pdev->dev,
@@ -4171,6 +4178,7 @@ void mpi3mr_memset_buffers(struct mpi3mr_ioc *mrioc)
 		memset(mrioc->admin_req_base, 0, mrioc->admin_req_q_sz);
 	if (mrioc->admin_reply_base)
 		memset(mrioc->admin_reply_base, 0, mrioc->admin_reply_q_sz);
+	atomic_set(&mrioc->admin_reply_q_in_use, 0);
 
 	if (mrioc->init_cmds.reply) {
 		memset(mrioc->init_cmds.reply, 0, sizeof(*mrioc->init_cmds.reply));
drivers/scsi/mpi3mr/mpi3mr_os.c +1 -0
index 3306de7170f6..2ba1b7641bc2 100644
@@ -3720,6 +3720,7 @@ int mpi3mr_issue_tm(struct mpi3mr_ioc *mrioc, u8 tm_type,
 		mpi3mr_poll_pend_io_completions(mrioc);
 		mpi3mr_ioc_enable_intr(mrioc);
 		mpi3mr_poll_pend_io_completions(mrioc);
+		mpi3mr_process_admin_reply_q(mrioc);
 	}
 	switch (tm_type) {
 	case MPI3_SCSITASKMGMT_TASKTYPE_TARGET_RESET:
Signature · 1 line
2.31.1
#2 lore raw

PATCH 2/6 mpi3mr: Driver unload crash host when enhanced logging is enabled

In reply to Ranjan Kumar

Driver was trying to access null pointer in a debug print while removing a device during driver unload

---
 drivers/scsi/mpi3mr/mpi3mr_transport.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/scsi/mpi3mr/mpi3mr_transport.c +2 -1
index 3fc897336b5e..584daf8a3ac9 100644
@@ -1552,7 +1552,8 @@ static void mpi3mr_sas_port_remove(struct mpi3mr_ioc *mrioc, u64 sas_address,
 
 	list_for_each_entry_safe(mr_sas_phy, next_phy,
 	    &mr_sas_port->phy_list, port_siblings) {
-		if ((mrioc->logging_level & MPI3_DEBUG_TRANSPORT_INFO))
+		if ((!mrioc->stop_drv_processing) &&
+		    (mrioc->logging_level & MPI3_DEBUG_TRANSPORT_INFO))
 			dev_info(&mr_sas_port->port->dev,
 			    "remove: sas_address(0x%016llx), phy(%d)\n",
 			    (unsigned long long)
Signature · 1 line
2.31.1
#3 lore raw

PATCH 3/6 mpi3mr: Wait for diagnostic save during controller init

In reply to Ranjan Kumar

If the controller reset operation is triggered to recover the controller from the fault state then wait for the snapdump to be saved in the firmware region before proceeding for resetting the controller.

---
 drivers/scsi/mpi3mr/mpi3mr_fw.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)
drivers/scsi/mpi3mr/mpi3mr_fw.c +18 -1
index e7ea3f7b9205..e5c0bc6d47d5 100644
@@ -1201,7 +1201,7 @@ mpi3mr_revalidate_factsdata(struct mpi3mr_ioc *mrioc)
  */
 static int mpi3mr_bring_ioc_ready(struct mpi3mr_ioc *mrioc)
 {
-	u32 ioc_config, ioc_status, timeout;
+	u32 ioc_config, ioc_status, timeout, host_diagnostic;
 	int retval = 0;
 	enum mpi3mr_iocstate ioc_state;
 	u64 base_info;
@@ -1255,6 +1255,23 @@ static int mpi3mr_bring_ioc_ready(struct mpi3mr_ioc *mrioc)
 			    retval, mpi3mr_iocstate_name(ioc_state));
 	}
 	if (ioc_state != MRIOC_STATE_RESET) {
+		if (ioc_state == MRIOC_STATE_FAULT) {
+			timeout = MPI3_SYSIF_DIAG_SAVE_TIMEOUT * 10;
+			mpi3mr_print_fault_info(mrioc);
+			do {
+				host_diagnostic =
+					readl(&mrioc->sysif_regs->host_diagnostic);
+				if (!(host_diagnostic &
+				      MPI3_SYSIF_HOST_DIAG_SAVE_IN_PROGRESS))
+					break;
+				if (!pci_device_is_present(mrioc->pdev)) {
+					mrioc->unrecoverable = 1;
+					ioc_err(mrioc, "controller is not present at the bringup\n");
+					goto out_device_not_present;
+				}
+				msleep(100);
+			} while (--timeout);
+		}
 		mpi3mr_print_fault_info(mrioc);
 		ioc_info(mrioc, "issuing soft reset to bring to reset state\n");
 		retval = mpi3mr_issue_reset(mrioc,
Signature · 1 line
2.31.1
#4 lore raw

PATCH 4/6 mpi3mr: appropriate return values for failures in firmware init path

In reply to Ranjan Kumar

The driver is modified to return a proper non-zero return value for all the cases when the controller initialization and re-initialization fails.

---
 drivers/scsi/mpi3mr/mpi3mr_fw.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)
drivers/scsi/mpi3mr/mpi3mr_fw.c +9 -2
index e5c0bc6d47d5..8f20dc87bebf 100644
@@ -3856,8 +3856,10 @@ int mpi3mr_init_ioc(struct mpi3mr_ioc *mrioc)
 	dprint_init(mrioc, "allocating config page buffers\n");
 	mrioc->cfg_page = dma_alloc_coherent(&mrioc->pdev->dev,
 	    MPI3MR_DEFAULT_CFG_PAGE_SZ, &mrioc->cfg_page_dma, GFP_KERNEL);
-	if (!mrioc->cfg_page)
+	if (!mrioc->cfg_page) {
+		retval = -1;
 		goto out_failed_noretry;
+	}
 
 	mrioc->cfg_page_sz = MPI3MR_DEFAULT_CFG_PAGE_SZ;
 
@@ -3919,8 +3921,10 @@ int mpi3mr_init_ioc(struct mpi3mr_ioc *mrioc)
 		dprint_init(mrioc, "allocating memory for throttle groups\n");
 		sz = sizeof(struct mpi3mr_throttle_group_info);
 		mrioc->throttle_groups = kcalloc(mrioc->num_io_throttle_group, sz, GFP_KERNEL);
-		if (!mrioc->throttle_groups)
+		if (!mrioc->throttle_groups) {
+			retval = -1;
 			goto out_failed_noretry;
+		}
 	}
 
 	retval = mpi3mr_enable_events(mrioc);
@@ -3940,6 +3944,7 @@ int mpi3mr_init_ioc(struct mpi3mr_ioc *mrioc)
 		mpi3mr_memset_buffers(mrioc);
 		goto retry_init;
 	}
+	retval = -1;
 out_failed_noretry:
 	ioc_err(mrioc, "controller initialization failed\n");
 	mpi3mr_issue_reset(mrioc, MPI3_SYSIF_HOST_DIAG_RESET_ACTION_DIAG_FAULT,
@@ -4052,6 +4057,7 @@ int mpi3mr_reinit_ioc(struct mpi3mr_ioc *mrioc, u8 is_resume)
 		ioc_err(mrioc,
 		    "cannot create minimum number of operational queues expected:%d created:%d\n",
 		    mrioc->shost->nr_hw_queues, mrioc->num_op_reply_q);
+		retval = -1;
 		goto out_failed_noretry;
 	}
 
@@ -4118,6 +4124,7 @@ int mpi3mr_reinit_ioc(struct mpi3mr_ioc *mrioc, u8 is_resume)
 		mpi3mr_memset_buffers(mrioc);
 		goto retry_init;
 	}
+	retval = -1;
 out_failed_noretry:
 	ioc_err(mrioc, "controller %s is failed\n",
 	    (is_resume)?"resume":"re-initialization");
Signature · 1 line
2.31.1
#5 lore raw

PATCH 5/6 mpi3mr: NVMe commands size greater than 8K fails

In reply to Ranjan Kumar

A wrong variable is checked while populating PRP entries in the PRP page and that results in failure .Hence no PRP entries in the PRP page were successfully created and any NVMe Encapsulated commands with PRP of size greater than 8K failed

---
 drivers/scsi/mpi3mr/mpi3mr_app.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/scsi/mpi3mr/mpi3mr_app.c +2 -2
index 9baac224b213..49318ba893c8 100644
@@ -894,7 +894,7 @@ static int mpi3mr_build_nvme_prp(struct mpi3mr_ioc *mrioc,
 			 * each time through the loop.
 			 */
 			*prp_entry = cpu_to_le64(dma_addr);
-			if (*prp1_entry & sgemod_mask) {
+			if (*prp_entry & sgemod_mask) {
 				dprint_bsg_err(mrioc,
 				    "%s: PRP address collides with SGE modifier\n",
 				    __func__);
@@ -903,7 +903,7 @@ static int mpi3mr_build_nvme_prp(struct mpi3mr_ioc *mrioc,
 			*prp_entry &= ~sgemod_mask;
 			*prp_entry |= sgemod_val;
 			prp_entry++;
-			prp_entry_dma++;
+			prp_entry_dma += prp_size;
 		}
 
 		/*
Signature · 1 line
2.31.1
#6 lore raw

PATCH 6/6 mpi3mr: Bad drive in topology results kernel crash

In reply to Ranjan Kumar

When the SAS Transport Layer support is enabled and when a device exposed to the OS by the driver failed Inquiry commands then the driver frees up the memory allocated for an internal HBA port data structure. However, in some places, the reference to the freed memory is not cleared.When the firmware sends the Device Info change event for the same device again,then the freed memory is accessed and that leads to memory corruption and OS crash.

---
 drivers/scsi/mpi3mr/mpi3mr_transport.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)
drivers/scsi/mpi3mr/mpi3mr_transport.c +7 -5
index 584daf8a3ac9..1869e45e04d4 100644
@@ -2358,15 +2358,16 @@ int mpi3mr_report_tgtdev_to_sas_transport(struct mpi3mr_ioc *mrioc,
 	tgtdev->host_exposed = 1;
 	if (!mpi3mr_sas_port_add(mrioc, tgtdev->dev_handle,
 	    sas_address_parent, hba_port)) {
-		tgtdev->host_exposed = 0;
 		retval = -1;
-	} else if ((!tgtdev->starget)) {
-		if (!mrioc->is_driver_loading)
+		} else if ((!tgtdev->starget) && (!mrioc->is_driver_loading)) {
 			mpi3mr_sas_port_remove(mrioc, sas_address,
 			    sas_address_parent, hba_port);
-		tgtdev->host_exposed = 0;
 		retval = -1;
 	}
+	if (retval) {
+		tgtdev->dev_spec.sas_sata_inf.hba_port = NULL;
+		tgtdev->host_exposed = 0;
+	}
 	return retval;
 }
 
@@ -2395,6 +2396,7 @@ void mpi3mr_remove_tgtdev_from_sas_transport(struct mpi3mr_ioc *mrioc,
 	mpi3mr_sas_port_remove(mrioc, sas_address, sas_address_parent,
 	    hba_port);
 	tgtdev->host_exposed = 0;
+	tgtdev->dev_spec.sas_sata_inf.hba_port = NULL;
 }
 
 /**
@@ -2451,7 +2453,7 @@ static u8 mpi3mr_get_port_id_by_rphy(struct mpi3mr_ioc *mrioc, struct sas_rphy *
 
 		tgtdev = __mpi3mr_get_tgtdev_by_addr_and_rphy(mrioc,
 			    rphy->identify.sas_address, rphy);
-		if (tgtdev) {
+		if (tgtdev && tgtdev->dev_spec.sas_sata_inf.hba_port) {
 			port_id =
 				tgtdev->dev_spec.sas_sata_inf.hba_port->port_id;
 			mpi3mr_tgtdev_put(tgtdev);
Signature · 1 line
2.31.1
#7 lore raw
In reply to Ranjan Kumar
Quoted 11 lines · Ranjan Kumar

This patchset contains critical Bug fixes

Ranjan Kumar (6):
mpi3mr: IOCTL timeout when disable/enable Interpt
mpi3mr: Driver unload crash host when enhanced logging is enabled
mpi3mr: Wait for diagnostic save during controller init
mpi3mr: appropriate return values for failures in firmware init path
mpi3mr: NVMe commands size greater than 8K fails
mpi3mr: Bad drive in topology results kernel crash

[...]

Applied to 6.3/scsi-fixes, thanks!

[1/6] mpi3mr: IOCTL timeout when disable/enable Interpt

      https://git.kernel.org/mkp/scsi/c/02ca7da2919a
[2/6] mpi3mr: Driver unload crash host when enhanced logging is enabled
      https://git.kernel.org/mkp/scsi/c/5b06a7169c59
[3/6] mpi3mr: Wait for diagnostic save during controller init
      https://git.kernel.org/mkp/scsi/c/0a319f162949
[4/6] mpi3mr: appropriate return values for failures in firmware init path
      https://git.kernel.org/mkp/scsi/c/ba8a9ba41fbd
[5/6] mpi3mr: NVMe commands size greater than 8K fails
      https://git.kernel.org/mkp/scsi/c/4f297e856a7b
[6/6] mpi3mr: Bad drive in topology results kernel crash
      https://git.kernel.org/mkp/scsi/c/8e45183978d6
Signature · 1 line
Martin K. Petersen	Oracle Linux Engineering
#8 lore raw

Hi,

Quoted 12 lines · Martin K. Petersen
Quoted 10 lines · Ranjan Kumar

This patchset contains critical Bug fixes

Ranjan Kumar (6):
mpi3mr: IOCTL timeout when disable/enable Interpt
mpi3mr: Driver unload crash host when enhanced logging is enabled
mpi3mr: Wait for diagnostic save during controller init
mpi3mr: appropriate return values for failures in firmware init path
mpi3mr: NVMe commands size greater than 8K fails
mpi3mr: Bad drive in topology results kernel crash

Quoted 3 lines
Quoted 1 line

[...]

Applied to 6.3/scsi-fixes, thanks!

Will those be backported as well as needed to at least 6.1.y where impacted? It was noticed that the patches do not contain Fixes tags and no CC to stable@ so they might not be picked automatically for stable series.

Regards,
Salvatore

#9 lore raw

Salvatore,

Quoted 8 lines
Quoted 8 lines
Quoted 8 lines

Ranjan Kumar (6):
mpi3mr: IOCTL timeout when disable/enable Interpt
mpi3mr: Driver unload crash host when enhanced logging is enabled
mpi3mr: Wait for diagnostic save during controller init
mpi3mr: appropriate return values for failures in firmware init path
mpi3mr: NVMe commands size greater than 8K fails
mpi3mr: Bad drive in topology results kernel crash

Quoted 8 lines
Quoted 3 lines
Quoted 1 line

[...]

Applied to 6.3/scsi-fixes, thanks!

Will those be backported as well as needed to at least 6.1.y where impacted? It was noticed that the patches do not contain Fixes tags and no CC to stable@ so they might not be picked automatically for stable series.

That's up to Broadcom. I rely on driver maintainers to tag the commits they wish to see in stable.

Signature · 1 line
Martin K. Petersen	Oracle Linux Engineering