All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/4] pm80xx updates
@ 2021-08-23  8:23 Ajish Koshy
  2021-08-23  8:23 ` [PATCH 1/4] scsi: pm80xx: fix incorrect port value when registering a device Ajish Koshy
                   ` (3 more replies)
  0 siblings, 4 replies; 11+ messages in thread
From: Ajish Koshy @ 2021-08-23  8:23 UTC (permalink / raw)
  To: linux-scsi
  Cc: Vasanthalakshmi.Tharmarajan, Viswas.G, Ruksar.devadi,
	Ashokkumar.N, Jinpu Wang

This patchset includes bugfixes for pm80xx driver.

Ajish Koshy (3):
  scsi: pm80xx: fix incorrect port value when registering a device
  scsi: pm80xx: fix lockup due to commit <1f02beff224e>
  scsi: pm80xx: fix memory leak during rmmod

Viswas G (1):
  scsi: pm80xx: Corrected Inbound and Outbound queue logging

 drivers/scsi/pm8001/pm8001_ctl.c  |  6 ++--
 drivers/scsi/pm8001/pm8001_hwi.c  |  7 +++-
 drivers/scsi/pm8001/pm8001_init.c | 12 +++++++
 drivers/scsi/pm8001/pm8001_sas.c  | 15 ++++++++
 drivers/scsi/pm8001/pm8001_sas.h  |  6 ++--
 drivers/scsi/pm8001/pm80xx_hwi.c  | 60 +++++++++++++++++++++++++------
 6 files changed, 91 insertions(+), 15 deletions(-)

-- 
2.27.0


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 1/4] scsi: pm80xx: fix incorrect port value when registering a device
  2021-08-23  8:23 [PATCH 0/4] pm80xx updates Ajish Koshy
@ 2021-08-23  8:23 ` Ajish Koshy
  2021-08-30  7:58   ` Jinpu Wang
  2021-08-23  8:23 ` [PATCH 2/4] scsi: pm80xx: fix lockup due to commit <1f02beff224e> Ajish Koshy
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 11+ messages in thread
From: Ajish Koshy @ 2021-08-23  8:23 UTC (permalink / raw)
  To: linux-scsi
  Cc: Vasanthalakshmi.Tharmarajan, Viswas.G, Ruksar.devadi,
	Ashokkumar.N, Jinpu Wang

Sometime we observe there is mismatch between portid & phyid
received during phyup event and device registration. Later this
will cause drive missing issue.

Signed-off-by: Ajish Koshy <Ajish.Koshy@microchip.com>
Signed-off-by: Viswas G <Viswas.G@microchip.com>
---
 drivers/scsi/pm8001/pm8001_hwi.c  |  7 ++++++-
 drivers/scsi/pm8001/pm8001_init.c |  1 +
 drivers/scsi/pm8001/pm8001_sas.c  | 15 +++++++++++++++
 drivers/scsi/pm8001/pm8001_sas.h  |  2 ++
 drivers/scsi/pm8001/pm80xx_hwi.c  |  7 ++++++-
 5 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/pm8001/pm8001_hwi.c b/drivers/scsi/pm8001/pm8001_hwi.c
index 63690508313b..c9ecddd0d719 100644
--- a/drivers/scsi/pm8001/pm8001_hwi.c
+++ b/drivers/scsi/pm8001/pm8001_hwi.c
@@ -3358,6 +3358,8 @@ hw_event_sas_phy_up(struct pm8001_hba_info *pm8001_ha, void *piomb)
 	struct pm8001_phy *phy = &pm8001_ha->phy[phy_id];
 	unsigned long flags;
 	u8 deviceType = pPayload->sas_identify.dev_type;
+	phy->port = port;
+	port->port_id = port_id;
 	port->port_state =  portstate;
 	phy->phy_state = PHY_STATE_LINK_UP_SPC;
 	pm8001_dbg(pm8001_ha, MSG,
@@ -3434,6 +3436,8 @@ hw_event_sata_phy_up(struct pm8001_hba_info *pm8001_ha, void *piomb)
 	unsigned long flags;
 	pm8001_dbg(pm8001_ha, DEVIO, "HW_EVENT_SATA_PHY_UP port id = %d, phy id = %d\n",
 		   port_id, phy_id);
+	phy->port = port;
+	port->port_id = port_id;
 	port->port_state =  portstate;
 	phy->phy_state = PHY_STATE_LINK_UP_SPC;
 	port->port_attached = 1;
@@ -4460,6 +4464,7 @@ static int pm8001_chip_reg_dev_req(struct pm8001_hba_info *pm8001_ha,
 	u16 ITNT = 2000;
 	struct domain_device *dev = pm8001_dev->sas_device;
 	struct domain_device *parent_dev = dev->parent;
+	struct pm8001_port *port = dev->port->lldd_port;
 	circularQ = &pm8001_ha->inbnd_q_tbl[0];
 
 	memset(&payload, 0, sizeof(payload));
@@ -4488,7 +4493,7 @@ static int pm8001_chip_reg_dev_req(struct pm8001_hba_info *pm8001_ha,
 	linkrate = (pm8001_dev->sas_device->linkrate < dev->port->linkrate) ?
 			pm8001_dev->sas_device->linkrate : dev->port->linkrate;
 	payload.phyid_portid =
-		cpu_to_le32(((pm8001_dev->sas_device->port->id) & 0x0F) |
+		cpu_to_le32(((port->port_id) & 0x0F) |
 		((phy_id & 0x0F) << 4));
 	payload.dtype_dlr_retry = cpu_to_le32((retryFlag & 0x01) |
 		((linkrate & 0x0F) * 0x1000000) |
diff --git a/drivers/scsi/pm8001/pm8001_init.c b/drivers/scsi/pm8001/pm8001_init.c
index 47db7e0beae6..613455a3e686 100644
--- a/drivers/scsi/pm8001/pm8001_init.c
+++ b/drivers/scsi/pm8001/pm8001_init.c
@@ -128,6 +128,7 @@ static struct sas_domain_function_template pm8001_transport_ops = {
 	.lldd_I_T_nexus_reset   = pm8001_I_T_nexus_reset,
 	.lldd_lu_reset		= pm8001_lu_reset,
 	.lldd_query_task	= pm8001_query_task,
+	.lldd_port_formed	= pm8001_port_formed,
 };
 
 /**
diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c
index 32e60f0c3b14..83e73009db5c 100644
--- a/drivers/scsi/pm8001/pm8001_sas.c
+++ b/drivers/scsi/pm8001/pm8001_sas.c
@@ -1355,3 +1355,18 @@ int pm8001_clear_task_set(struct domain_device *dev, u8 *lun)
 	tmf_task.tmf = TMF_CLEAR_TASK_SET;
 	return pm8001_issue_ssp_tmf(dev, lun, &tmf_task);
 }
+
+void pm8001_port_formed(struct asd_sas_phy *sas_phy)
+{
+	struct sas_ha_struct *sas_ha = sas_phy->ha;
+	struct pm8001_hba_info *pm8001_ha = sas_ha->lldd_ha;
+	struct pm8001_phy *phy = sas_phy->lldd_phy;
+	struct asd_sas_port *sas_port = sas_phy->port;
+	struct pm8001_port *port = phy->port;
+
+	if (!sas_port) {
+		pm8001_dbg(pm8001_ha, FAIL, "Received null port\n");
+		return;
+	}
+	sas_port->lldd_port = port;
+}
diff --git a/drivers/scsi/pm8001/pm8001_sas.h b/drivers/scsi/pm8001/pm8001_sas.h
index 62d08b535a4b..1a016a421280 100644
--- a/drivers/scsi/pm8001/pm8001_sas.h
+++ b/drivers/scsi/pm8001/pm8001_sas.h
@@ -230,6 +230,7 @@ struct pm8001_port {
 	u8			port_attached;
 	u16			wide_port_phymap;
 	u8			port_state;
+	u8			port_id;
 	struct list_head	list;
 };
 
@@ -651,6 +652,7 @@ int pm8001_lu_reset(struct domain_device *dev, u8 *lun);
 int pm8001_I_T_nexus_reset(struct domain_device *dev);
 int pm8001_I_T_nexus_event_handler(struct domain_device *dev);
 int pm8001_query_task(struct sas_task *task);
+void pm8001_port_formed(struct asd_sas_phy *sas_phy);
 void pm8001_open_reject_retry(
 	struct pm8001_hba_info *pm8001_ha,
 	struct sas_task *task_to_close,
diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c b/drivers/scsi/pm8001/pm80xx_hwi.c
index 6ffe17b849ae..cec932f830b8 100644
--- a/drivers/scsi/pm8001/pm80xx_hwi.c
+++ b/drivers/scsi/pm8001/pm80xx_hwi.c
@@ -3299,6 +3299,8 @@ hw_event_sas_phy_up(struct pm8001_hba_info *pm8001_ha, void *piomb)
 	struct pm8001_phy *phy = &pm8001_ha->phy[phy_id];
 	unsigned long flags;
 	u8 deviceType = pPayload->sas_identify.dev_type;
+	phy->port = port;
+	port->port_id = port_id;
 	port->port_state = portstate;
 	port->wide_port_phymap |= (1U << phy_id);
 	phy->phy_state = PHY_STATE_LINK_UP_SPCV;
@@ -3380,6 +3382,8 @@ hw_event_sata_phy_up(struct pm8001_hba_info *pm8001_ha, void *piomb)
 		   "port id %d, phy id %d link_rate %d portstate 0x%x\n",
 		   port_id, phy_id, link_rate, portstate);
 
+	phy->port = port;
+	port->port_id = port_id;
 	port->port_state = portstate;
 	phy->phy_state = PHY_STATE_LINK_UP_SPCV;
 	port->port_attached = 1;
@@ -4808,6 +4812,7 @@ static int pm80xx_chip_reg_dev_req(struct pm8001_hba_info *pm8001_ha,
 	u16 ITNT = 2000;
 	struct domain_device *dev = pm8001_dev->sas_device;
 	struct domain_device *parent_dev = dev->parent;
+	struct pm8001_port *port = dev->port->lldd_port;
 	circularQ = &pm8001_ha->inbnd_q_tbl[0];
 
 	memset(&payload, 0, sizeof(payload));
@@ -4840,7 +4845,7 @@ static int pm80xx_chip_reg_dev_req(struct pm8001_hba_info *pm8001_ha,
 			pm8001_dev->sas_device->linkrate : dev->port->linkrate;
 
 	payload.phyid_portid =
-		cpu_to_le32(((pm8001_dev->sas_device->port->id) & 0xFF) |
+		cpu_to_le32(((port->port_id) & 0xFF) |
 		((phy_id & 0xFF) << 8));
 
 	payload.dtype_dlr_mcn_ir_retry = cpu_to_le32((retryFlag & 0x01) |
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/4] scsi: pm80xx: fix lockup due to commit <1f02beff224e>
  2021-08-23  8:23 [PATCH 0/4] pm80xx updates Ajish Koshy
  2021-08-23  8:23 ` [PATCH 1/4] scsi: pm80xx: fix incorrect port value when registering a device Ajish Koshy
@ 2021-08-23  8:23 ` Ajish Koshy
  2021-08-30  8:22   ` Jinpu Wang
  2021-08-23  8:23 ` [PATCH 3/4] scsi: pm80xx: Corrected Inbound and Outbound queue logging Ajish Koshy
  2021-08-23  8:23 ` [PATCH 4/4] scsi: pm80xx: fix memory leak during rmmod Ajish Koshy
  3 siblings, 1 reply; 11+ messages in thread
From: Ajish Koshy @ 2021-08-23  8:23 UTC (permalink / raw)
  To: linux-scsi
  Cc: Vasanthalakshmi.Tharmarajan, Viswas.G, Ruksar.devadi,
	Ashokkumar.N, Jinpu Wang

Upstream commit <1f02beff224e> introduced a lock per outbound
queue, where the driver before that was using a global lock
for all outbound queues. While processing the IO responses
and events, driver takes the outbound queue spinlock and
later it is supposed to release the same spin lock in
pm8001_ccb_task_free_done() before calling command done().
Since the older code was using a global lock,
pm8001_ccb_task_free_done() was also releasing the global
spin lock. With the commit <1f02beff224e>,
pm8001_ccb_task_free_done() remains the same and it was
still using the global lock.

So when driver completes a SATA command,the global spinlock
will be in a locked state.
mpi_sata_completion()->spin_lock(&pm8001_ha->lock);

Later when driver gets a scsi command for SATA drive,
pm8001_task_exec() tries to acquire the global lock and leads
to lockup crash.

Signed-off-by: Ajish Koshy <Ajish.Koshy@microchip.com>
Signed-off-by: Viswas G <Viswas.G@microchip.com>
---
 drivers/scsi/pm8001/pm8001_sas.h |  3 +-
 drivers/scsi/pm8001/pm80xx_hwi.c | 53 ++++++++++++++++++++++++++------
 2 files changed, 45 insertions(+), 11 deletions(-)

diff --git a/drivers/scsi/pm8001/pm8001_sas.h b/drivers/scsi/pm8001/pm8001_sas.h
index 1a016a421280..3274d88a9ccc 100644
--- a/drivers/scsi/pm8001/pm8001_sas.h
+++ b/drivers/scsi/pm8001/pm8001_sas.h
@@ -458,6 +458,7 @@ struct outbound_queue_table {
 	__le32			producer_index;
 	u32			consumer_idx;
 	spinlock_t		oq_lock;
+	unsigned long		lock_flags;
 };
 struct pm8001_hba_memspace {
 	void __iomem  		*memvirtaddr;
@@ -740,9 +741,7 @@ pm8001_ccb_task_free_done(struct pm8001_hba_info *pm8001_ha,
 {
 	pm8001_ccb_task_free(pm8001_ha, task, ccb, ccb_idx);
 	smp_mb(); /*in order to force CPU ordering*/
-	spin_unlock(&pm8001_ha->lock);
 	task->task_done(task);
-	spin_lock(&pm8001_ha->lock);
 }
 
 #endif
diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c b/drivers/scsi/pm8001/pm80xx_hwi.c
index cec932f830b8..1ae2f5c6042c 100644
--- a/drivers/scsi/pm8001/pm80xx_hwi.c
+++ b/drivers/scsi/pm8001/pm80xx_hwi.c
@@ -2379,7 +2379,8 @@ static void mpi_ssp_event(struct pm8001_hba_info *pm8001_ha, void *piomb)
 
 /*See the comments for mpi_ssp_completion */
 static void
-mpi_sata_completion(struct pm8001_hba_info *pm8001_ha, void *piomb)
+mpi_sata_completion(struct pm8001_hba_info *pm8001_ha,
+		struct outbound_queue_table *circularQ, void *piomb)
 {
 	struct sas_task *t;
 	struct pm8001_ccb_info *ccb;
@@ -2616,7 +2617,11 @@ mpi_sata_completion(struct pm8001_hba_info *pm8001_ha, void *piomb)
 				IO_OPEN_CNX_ERROR_IT_NEXUS_LOSS);
 			ts->resp = SAS_TASK_UNDELIVERED;
 			ts->stat = SAS_QUEUE_FULL;
+			spin_unlock_irqrestore(&circularQ->oq_lock,
+					circularQ->lock_flags);
 			pm8001_ccb_task_free_done(pm8001_ha, t, ccb, tag);
+			spin_lock_irqsave(&circularQ->oq_lock,
+					circularQ->lock_flags);
 			return;
 		}
 		break;
@@ -2632,7 +2637,11 @@ mpi_sata_completion(struct pm8001_hba_info *pm8001_ha, void *piomb)
 				IO_OPEN_CNX_ERROR_IT_NEXUS_LOSS);
 			ts->resp = SAS_TASK_UNDELIVERED;
 			ts->stat = SAS_QUEUE_FULL;
+			spin_unlock_irqrestore(&circularQ->oq_lock,
+					circularQ->lock_flags);
 			pm8001_ccb_task_free_done(pm8001_ha, t, ccb, tag);
+			spin_lock_irqsave(&circularQ->oq_lock,
+					circularQ->lock_flags);
 			return;
 		}
 		break;
@@ -2656,7 +2665,11 @@ mpi_sata_completion(struct pm8001_hba_info *pm8001_ha, void *piomb)
 				IO_OPEN_CNX_ERROR_STP_RESOURCES_BUSY);
 			ts->resp = SAS_TASK_UNDELIVERED;
 			ts->stat = SAS_QUEUE_FULL;
+			spin_unlock_irqrestore(&circularQ->oq_lock,
+					circularQ->lock_flags);
 			pm8001_ccb_task_free_done(pm8001_ha, t, ccb, tag);
+			spin_lock_irqsave(&circularQ->oq_lock,
+					circularQ->lock_flags);
 			return;
 		}
 		break;
@@ -2727,7 +2740,11 @@ mpi_sata_completion(struct pm8001_hba_info *pm8001_ha, void *piomb)
 					IO_DS_NON_OPERATIONAL);
 			ts->resp = SAS_TASK_UNDELIVERED;
 			ts->stat = SAS_QUEUE_FULL;
+			spin_unlock_irqrestore(&circularQ->oq_lock,
+					circularQ->lock_flags);
 			pm8001_ccb_task_free_done(pm8001_ha, t, ccb, tag);
+			spin_lock_irqsave(&circularQ->oq_lock,
+					circularQ->lock_flags);
 			return;
 		}
 		break;
@@ -2747,7 +2764,11 @@ mpi_sata_completion(struct pm8001_hba_info *pm8001_ha, void *piomb)
 					IO_DS_IN_ERROR);
 			ts->resp = SAS_TASK_UNDELIVERED;
 			ts->stat = SAS_QUEUE_FULL;
+			spin_unlock_irqrestore(&circularQ->oq_lock,
+					circularQ->lock_flags);
 			pm8001_ccb_task_free_done(pm8001_ha, t, ccb, tag);
+			spin_lock_irqsave(&circularQ->oq_lock,
+					circularQ->lock_flags);
 			return;
 		}
 		break;
@@ -2785,12 +2806,17 @@ mpi_sata_completion(struct pm8001_hba_info *pm8001_ha, void *piomb)
 		pm8001_ccb_task_free(pm8001_ha, t, ccb, tag);
 	} else {
 		spin_unlock_irqrestore(&t->task_state_lock, flags);
+		spin_unlock_irqrestore(&circularQ->oq_lock,
+				circularQ->lock_flags);
 		pm8001_ccb_task_free_done(pm8001_ha, t, ccb, tag);
+		spin_lock_irqsave(&circularQ->oq_lock,
+				circularQ->lock_flags);
 	}
 }
 
 /*See the comments for mpi_ssp_completion */
-static void mpi_sata_event(struct pm8001_hba_info *pm8001_ha, void *piomb)
+static void mpi_sata_event(struct pm8001_hba_info *pm8001_ha,
+		struct outbound_queue_table *circularQ, void *piomb)
 {
 	struct sas_task *t;
 	struct task_status_struct *ts;
@@ -2890,7 +2916,11 @@ static void mpi_sata_event(struct pm8001_hba_info *pm8001_ha, void *piomb)
 				IO_OPEN_CNX_ERROR_IT_NEXUS_LOSS);
 			ts->resp = SAS_TASK_COMPLETE;
 			ts->stat = SAS_QUEUE_FULL;
+			spin_unlock_irqrestore(&circularQ->oq_lock,
+					circularQ->lock_flags);
 			pm8001_ccb_task_free_done(pm8001_ha, t, ccb, tag);
+			spin_lock_irqsave(&circularQ->oq_lock,
+					circularQ->lock_flags);
 			return;
 		}
 		break;
@@ -3002,7 +3032,11 @@ static void mpi_sata_event(struct pm8001_hba_info *pm8001_ha, void *piomb)
 		pm8001_ccb_task_free(pm8001_ha, t, ccb, tag);
 	} else {
 		spin_unlock_irqrestore(&t->task_state_lock, flags);
+		spin_unlock_irqrestore(&circularQ->oq_lock,
+				circularQ->lock_flags);
 		pm8001_ccb_task_free_done(pm8001_ha, t, ccb, tag);
+		spin_lock_irqsave(&circularQ->oq_lock,
+				circularQ->lock_flags);
 	}
 }
 
@@ -3906,7 +3940,8 @@ static int ssp_coalesced_comp_resp(struct pm8001_hba_info *pm8001_ha,
  * @pm8001_ha: our hba card information
  * @piomb: IO message buffer
  */
-static void process_one_iomb(struct pm8001_hba_info *pm8001_ha, void *piomb)
+static void process_one_iomb(struct pm8001_hba_info *pm8001_ha,
+		struct outbound_queue_table *circularQ, void *piomb)
 {
 	__le32 pHeader = *(__le32 *)piomb;
 	u32 opc = (u32)((le32_to_cpu(pHeader)) & 0xFFF);
@@ -3948,11 +3983,11 @@ static void process_one_iomb(struct pm8001_hba_info *pm8001_ha, void *piomb)
 		break;
 	case OPC_OUB_SATA_COMP:
 		pm8001_dbg(pm8001_ha, MSG, "OPC_OUB_SATA_COMP\n");
-		mpi_sata_completion(pm8001_ha, piomb);
+		mpi_sata_completion(pm8001_ha, circularQ, piomb);
 		break;
 	case OPC_OUB_SATA_EVENT:
 		pm8001_dbg(pm8001_ha, MSG, "OPC_OUB_SATA_EVENT\n");
-		mpi_sata_event(pm8001_ha, piomb);
+		mpi_sata_event(pm8001_ha, circularQ, piomb);
 		break;
 	case OPC_OUB_SSP_EVENT:
 		pm8001_dbg(pm8001_ha, MSG, "OPC_OUB_SSP_EVENT\n");
@@ -4121,7 +4156,6 @@ static int process_oq(struct pm8001_hba_info *pm8001_ha, u8 vec)
 	void *pMsg1 = NULL;
 	u8 bc;
 	u32 ret = MPI_IO_STATUS_FAIL;
-	unsigned long flags;
 	u32 regval;
 
 	if (vec == (pm8001_ha->max_q_num - 1)) {
@@ -4138,7 +4172,7 @@ static int process_oq(struct pm8001_hba_info *pm8001_ha, u8 vec)
 		}
 	}
 	circularQ = &pm8001_ha->outbnd_q_tbl[vec];
-	spin_lock_irqsave(&circularQ->oq_lock, flags);
+	spin_lock_irqsave(&circularQ->oq_lock, circularQ->lock_flags);
 	do {
 		/* spurious interrupt during setup if kexec-ing and
 		 * driver doing a doorbell access w/ the pre-kexec oq
@@ -4149,7 +4183,8 @@ static int process_oq(struct pm8001_hba_info *pm8001_ha, u8 vec)
 		ret = pm8001_mpi_msg_consume(pm8001_ha, circularQ, &pMsg1, &bc);
 		if (MPI_IO_STATUS_SUCCESS == ret) {
 			/* process the outbound message */
-			process_one_iomb(pm8001_ha, (void *)(pMsg1 - 4));
+			process_one_iomb(pm8001_ha, circularQ,
+						(void *)(pMsg1 - 4));
 			/* free the message from the outbound circular buffer */
 			pm8001_mpi_msg_free_set(pm8001_ha, pMsg1,
 							circularQ, bc);
@@ -4164,7 +4199,7 @@ static int process_oq(struct pm8001_hba_info *pm8001_ha, u8 vec)
 				break;
 		}
 	} while (1);
-	spin_unlock_irqrestore(&circularQ->oq_lock, flags);
+	spin_unlock_irqrestore(&circularQ->oq_lock, circularQ->lock_flags);
 	return ret;
 }
 
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 3/4] scsi: pm80xx: Corrected Inbound and Outbound queue logging
  2021-08-23  8:23 [PATCH 0/4] pm80xx updates Ajish Koshy
  2021-08-23  8:23 ` [PATCH 1/4] scsi: pm80xx: fix incorrect port value when registering a device Ajish Koshy
  2021-08-23  8:23 ` [PATCH 2/4] scsi: pm80xx: fix lockup due to commit <1f02beff224e> Ajish Koshy
@ 2021-08-23  8:23 ` Ajish Koshy
  2021-08-30  8:23   ` Jinpu Wang
  2021-08-23  8:23 ` [PATCH 4/4] scsi: pm80xx: fix memory leak during rmmod Ajish Koshy
  3 siblings, 1 reply; 11+ messages in thread
From: Ajish Koshy @ 2021-08-23  8:23 UTC (permalink / raw)
  To: linux-scsi
  Cc: Vasanthalakshmi.Tharmarajan, Viswas.G, Ruksar.devadi,
	Ashokkumar.N, Jinpu Wang

From: Viswas G <Viswas.G@microchip.com>

Corrected inbound queue and outbound queue size in 'ib_log'
and 'ob_log' sysfs entries.

Signed-off-by: Viswas G <Viswas.G@microchip.com>
Signed-off-by: Ajish Koshy <Ajish.Koshy@microchip.com>
---
 drivers/scsi/pm8001/pm8001_ctl.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/pm8001/pm8001_ctl.c b/drivers/scsi/pm8001/pm8001_ctl.c
index ec05c42e8ee6..b25e447aa3bd 100644
--- a/drivers/scsi/pm8001/pm8001_ctl.c
+++ b/drivers/scsi/pm8001/pm8001_ctl.c
@@ -409,6 +409,7 @@ static ssize_t pm8001_ctl_ib_queue_log_show(struct device *cdev,
 	char *str = buf;
 	int start = 0;
 	u32 ib_offset = pm8001_ha->ib_offset;
+	u32 queue_size = pm8001_ha->max_q_num * PM8001_MPI_QUEUE * 128;
 #define IB_MEMMAP(c)	\
 		(*(u32 *)((u8 *)pm8001_ha->	\
 		memoryMap.region[ib_offset].virt_ptr +	\
@@ -419,7 +420,7 @@ static ssize_t pm8001_ctl_ib_queue_log_show(struct device *cdev,
 		start = start + 4;
 	}
 	pm8001_ha->evtlog_ib_offset += SYSFS_OFFSET;
-	if (((pm8001_ha->evtlog_ib_offset) % (PM80XX_IB_OB_QUEUE_SIZE)) == 0)
+	if (((pm8001_ha->evtlog_ib_offset) % queue_size) == 0)
 		pm8001_ha->evtlog_ib_offset = 0;
 
 	return str - buf;
@@ -445,6 +446,7 @@ static ssize_t pm8001_ctl_ob_queue_log_show(struct device *cdev,
 	char *str = buf;
 	int start = 0;
 	u32 ob_offset = pm8001_ha->ob_offset;
+	u32 queue_size = pm8001_ha->max_q_num * PM8001_MPI_QUEUE * 128;
 #define OB_MEMMAP(c)	\
 		(*(u32 *)((u8 *)pm8001_ha->	\
 		memoryMap.region[ob_offset].virt_ptr +	\
@@ -455,7 +457,7 @@ static ssize_t pm8001_ctl_ob_queue_log_show(struct device *cdev,
 		start = start + 4;
 	}
 	pm8001_ha->evtlog_ob_offset += SYSFS_OFFSET;
-	if (((pm8001_ha->evtlog_ob_offset) % (PM80XX_IB_OB_QUEUE_SIZE)) == 0)
+	if (((pm8001_ha->evtlog_ob_offset) % queue_size) == 0)
 		pm8001_ha->evtlog_ob_offset = 0;
 
 	return str - buf;
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 4/4] scsi: pm80xx: fix memory leak during rmmod
  2021-08-23  8:23 [PATCH 0/4] pm80xx updates Ajish Koshy
                   ` (2 preceding siblings ...)
  2021-08-23  8:23 ` [PATCH 3/4] scsi: pm80xx: Corrected Inbound and Outbound queue logging Ajish Koshy
@ 2021-08-23  8:23 ` Ajish Koshy
  2021-08-30  8:25   ` Jinpu Wang
  3 siblings, 1 reply; 11+ messages in thread
From: Ajish Koshy @ 2021-08-23  8:23 UTC (permalink / raw)
  To: linux-scsi
  Cc: Vasanthalakshmi.Tharmarajan, Viswas.G, Ruksar.devadi,
	Ashokkumar.N, Jinpu Wang

Driver fails to release memory allocated. This will lead
to memory leak during driver removal.

Signed-off-by: Ajish Koshy <Ajish.Koshy@microchip.com>
Signed-off-by: Viswas G <Viswas.G@microchip.com>
---
 drivers/scsi/pm8001/pm8001_init.c | 11 +++++++++++
 drivers/scsi/pm8001/pm8001_sas.h  |  1 +
 2 files changed, 12 insertions(+)

diff --git a/drivers/scsi/pm8001/pm8001_init.c b/drivers/scsi/pm8001/pm8001_init.c
index 613455a3e686..7082fecf7ce8 100644
--- a/drivers/scsi/pm8001/pm8001_init.c
+++ b/drivers/scsi/pm8001/pm8001_init.c
@@ -1199,6 +1199,7 @@ pm8001_init_ccb_tag(struct pm8001_hba_info *pm8001_ha, struct Scsi_Host *shost,
 		goto err_out;
 
 	/* Memory region for ccb_info*/
+	pm8001_ha->ccb_count = ccb_count;
 	pm8001_ha->ccb_info =
 		kcalloc(ccb_count, sizeof(struct pm8001_ccb_info), GFP_KERNEL);
 	if (!pm8001_ha->ccb_info) {
@@ -1260,6 +1261,16 @@ static void pm8001_pci_remove(struct pci_dev *pdev)
 			tasklet_kill(&pm8001_ha->tasklet[j]);
 #endif
 	scsi_host_put(pm8001_ha->shost);
+
+	for (i = 0; i < pm8001_ha->ccb_count; i++) {
+		dma_free_coherent(&pm8001_ha->pdev->dev,
+			sizeof(struct pm8001_prd) * PM8001_MAX_DMA_SG,
+			pm8001_ha->ccb_info[i].buf_prd,
+			pm8001_ha->ccb_info[i].ccb_dma_handle);
+	}
+	kfree(pm8001_ha->ccb_info);
+	kfree(pm8001_ha->devices);
+
 	pm8001_free(pm8001_ha);
 	kfree(sha->sas_phy);
 	kfree(sha->sas_port);
diff --git a/drivers/scsi/pm8001/pm8001_sas.h b/drivers/scsi/pm8001/pm8001_sas.h
index 3274d88a9ccc..7e999768bfd2 100644
--- a/drivers/scsi/pm8001/pm8001_sas.h
+++ b/drivers/scsi/pm8001/pm8001_sas.h
@@ -518,6 +518,7 @@ struct pm8001_hba_info {
 	u32			iomb_size; /* SPC and SPCV IOMB size */
 	struct pm8001_device	*devices;
 	struct pm8001_ccb_info	*ccb_info;
+	u32			ccb_count;
 #ifdef PM8001_USE_MSIX
 	int			number_of_intr;/*will be used in remove()*/
 	char			intr_drvname[PM8001_MAX_MSIX_VEC]
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/4] scsi: pm80xx: fix incorrect port value when registering a device
  2021-08-23  8:23 ` [PATCH 1/4] scsi: pm80xx: fix incorrect port value when registering a device Ajish Koshy
@ 2021-08-30  7:58   ` Jinpu Wang
  2021-09-01 11:44     ` Ajish.Koshy
  0 siblings, 1 reply; 11+ messages in thread
From: Jinpu Wang @ 2021-08-30  7:58 UTC (permalink / raw)
  To: Ajish Koshy
  Cc: Linux SCSI Mailinglist, Vasanthalakshmi.Tharmarajan, Viswas G,
	Ruksar Devadi, Ashokkumar N, Jinpu Wang

Hi Ajish,
On Mon, Aug 23, 2021 at 9:28 AM Ajish Koshy <Ajish.Koshy@microchip.com> wrote:
>
> Sometime we observe there is mismatch between portid & phyid
> received during phyup event and device registration. Later this
> will cause drive missing issue.
Thanks for the patch.
Could you explain a bit more, what was the problem, and how you fix it
in the commit message?
>
> Signed-off-by: Ajish Koshy <Ajish.Koshy@microchip.com>
> Signed-off-by: Viswas G <Viswas.G@microchip.com>
> ---
>  drivers/scsi/pm8001/pm8001_hwi.c  |  7 ++++++-
>  drivers/scsi/pm8001/pm8001_init.c |  1 +
>  drivers/scsi/pm8001/pm8001_sas.c  | 15 +++++++++++++++
>  drivers/scsi/pm8001/pm8001_sas.h  |  2 ++
>  drivers/scsi/pm8001/pm80xx_hwi.c  |  7 ++++++-
>  5 files changed, 30 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/pm8001/pm8001_hwi.c b/drivers/scsi/pm8001/pm8001_hwi.c
> index 63690508313b..c9ecddd0d719 100644
> --- a/drivers/scsi/pm8001/pm8001_hwi.c
> +++ b/drivers/scsi/pm8001/pm8001_hwi.c
> @@ -3358,6 +3358,8 @@ hw_event_sas_phy_up(struct pm8001_hba_info *pm8001_ha, void *piomb)
>         struct pm8001_phy *phy = &pm8001_ha->phy[phy_id];
>         unsigned long flags;
>         u8 deviceType = pPayload->sas_identify.dev_type;
> +       phy->port = port;
> +       port->port_id = port_id;
>         port->port_state =  portstate;
>         phy->phy_state = PHY_STATE_LINK_UP_SPC;
>         pm8001_dbg(pm8001_ha, MSG,
> @@ -3434,6 +3436,8 @@ hw_event_sata_phy_up(struct pm8001_hba_info *pm8001_ha, void *piomb)
>         unsigned long flags;
>         pm8001_dbg(pm8001_ha, DEVIO, "HW_EVENT_SATA_PHY_UP port id = %d, phy id = %d\n",
>                    port_id, phy_id);
> +       phy->port = port;
> +       port->port_id = port_id;
>         port->port_state =  portstate;
>         phy->phy_state = PHY_STATE_LINK_UP_SPC;
>         port->port_attached = 1;
> @@ -4460,6 +4464,7 @@ static int pm8001_chip_reg_dev_req(struct pm8001_hba_info *pm8001_ha,
>         u16 ITNT = 2000;
>         struct domain_device *dev = pm8001_dev->sas_device;
>         struct domain_device *parent_dev = dev->parent;
> +       struct pm8001_port *port = dev->port->lldd_port;
>         circularQ = &pm8001_ha->inbnd_q_tbl[0];
>
>         memset(&payload, 0, sizeof(payload));
> @@ -4488,7 +4493,7 @@ static int pm8001_chip_reg_dev_req(struct pm8001_hba_info *pm8001_ha,
>         linkrate = (pm8001_dev->sas_device->linkrate < dev->port->linkrate) ?
>                         pm8001_dev->sas_device->linkrate : dev->port->linkrate;
>         payload.phyid_portid =
> -               cpu_to_le32(((pm8001_dev->sas_device->port->id) & 0x0F) |
> +               cpu_to_le32(((port->port_id) & 0x0F) |
>                 ((phy_id & 0x0F) << 4));
>         payload.dtype_dlr_retry = cpu_to_le32((retryFlag & 0x01) |
>                 ((linkrate & 0x0F) * 0x1000000) |
> diff --git a/drivers/scsi/pm8001/pm8001_init.c b/drivers/scsi/pm8001/pm8001_init.c
> index 47db7e0beae6..613455a3e686 100644
> --- a/drivers/scsi/pm8001/pm8001_init.c
> +++ b/drivers/scsi/pm8001/pm8001_init.c
> @@ -128,6 +128,7 @@ static struct sas_domain_function_template pm8001_transport_ops = {
>         .lldd_I_T_nexus_reset   = pm8001_I_T_nexus_reset,
>         .lldd_lu_reset          = pm8001_lu_reset,
>         .lldd_query_task        = pm8001_query_task,
> +       .lldd_port_formed       = pm8001_port_formed,
>  };
>
>  /**
> diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c
> index 32e60f0c3b14..83e73009db5c 100644
> --- a/drivers/scsi/pm8001/pm8001_sas.c
> +++ b/drivers/scsi/pm8001/pm8001_sas.c
> @@ -1355,3 +1355,18 @@ int pm8001_clear_task_set(struct domain_device *dev, u8 *lun)
>         tmf_task.tmf = TMF_CLEAR_TASK_SET;
>         return pm8001_issue_ssp_tmf(dev, lun, &tmf_task);
>  }
> +
> +void pm8001_port_formed(struct asd_sas_phy *sas_phy)
> +{
> +       struct sas_ha_struct *sas_ha = sas_phy->ha;
> +       struct pm8001_hba_info *pm8001_ha = sas_ha->lldd_ha;
> +       struct pm8001_phy *phy = sas_phy->lldd_phy;
> +       struct asd_sas_port *sas_port = sas_phy->port;
> +       struct pm8001_port *port = phy->port;
> +
> +       if (!sas_port) {
> +               pm8001_dbg(pm8001_ha, FAIL, "Received null port\n");
> +               return;
> +       }
> +       sas_port->lldd_port = port;
> +}
> diff --git a/drivers/scsi/pm8001/pm8001_sas.h b/drivers/scsi/pm8001/pm8001_sas.h
> index 62d08b535a4b..1a016a421280 100644
> --- a/drivers/scsi/pm8001/pm8001_sas.h
> +++ b/drivers/scsi/pm8001/pm8001_sas.h
> @@ -230,6 +230,7 @@ struct pm8001_port {
>         u8                      port_attached;
>         u16                     wide_port_phymap;
>         u8                      port_state;
> +       u8                      port_id;
>         struct list_head        list;
>  };
>
> @@ -651,6 +652,7 @@ int pm8001_lu_reset(struct domain_device *dev, u8 *lun);
>  int pm8001_I_T_nexus_reset(struct domain_device *dev);
>  int pm8001_I_T_nexus_event_handler(struct domain_device *dev);
>  int pm8001_query_task(struct sas_task *task);
> +void pm8001_port_formed(struct asd_sas_phy *sas_phy);
>  void pm8001_open_reject_retry(
>         struct pm8001_hba_info *pm8001_ha,
>         struct sas_task *task_to_close,
> diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c b/drivers/scsi/pm8001/pm80xx_hwi.c
> index 6ffe17b849ae..cec932f830b8 100644
> --- a/drivers/scsi/pm8001/pm80xx_hwi.c
> +++ b/drivers/scsi/pm8001/pm80xx_hwi.c
> @@ -3299,6 +3299,8 @@ hw_event_sas_phy_up(struct pm8001_hba_info *pm8001_ha, void *piomb)
>         struct pm8001_phy *phy = &pm8001_ha->phy[phy_id];
>         unsigned long flags;
>         u8 deviceType = pPayload->sas_identify.dev_type;
> +       phy->port = port;
> +       port->port_id = port_id;
>         port->port_state = portstate;
>         port->wide_port_phymap |= (1U << phy_id);
>         phy->phy_state = PHY_STATE_LINK_UP_SPCV;
> @@ -3380,6 +3382,8 @@ hw_event_sata_phy_up(struct pm8001_hba_info *pm8001_ha, void *piomb)
>                    "port id %d, phy id %d link_rate %d portstate 0x%x\n",
>                    port_id, phy_id, link_rate, portstate);
>
> +       phy->port = port;
> +       port->port_id = port_id;
>         port->port_state = portstate;
>         phy->phy_state = PHY_STATE_LINK_UP_SPCV;
>         port->port_attached = 1;
> @@ -4808,6 +4812,7 @@ static int pm80xx_chip_reg_dev_req(struct pm8001_hba_info *pm8001_ha,
>         u16 ITNT = 2000;
>         struct domain_device *dev = pm8001_dev->sas_device;
>         struct domain_device *parent_dev = dev->parent;
> +       struct pm8001_port *port = dev->port->lldd_port;
>         circularQ = &pm8001_ha->inbnd_q_tbl[0];
>
>         memset(&payload, 0, sizeof(payload));
> @@ -4840,7 +4845,7 @@ static int pm80xx_chip_reg_dev_req(struct pm8001_hba_info *pm8001_ha,
>                         pm8001_dev->sas_device->linkrate : dev->port->linkrate;
>
>         payload.phyid_portid =
> -               cpu_to_le32(((pm8001_dev->sas_device->port->id) & 0xFF) |
> +               cpu_to_le32(((port->port_id) & 0xFF) |
>                 ((phy_id & 0xFF) << 8));
>
>         payload.dtype_dlr_mcn_ir_retry = cpu_to_le32((retryFlag & 0x01) |
> --
> 2.27.0
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/4] scsi: pm80xx: fix lockup due to commit <1f02beff224e>
  2021-08-23  8:23 ` [PATCH 2/4] scsi: pm80xx: fix lockup due to commit <1f02beff224e> Ajish Koshy
@ 2021-08-30  8:22   ` Jinpu Wang
  2021-09-01 11:55     ` Ajish.Koshy
  0 siblings, 1 reply; 11+ messages in thread
From: Jinpu Wang @ 2021-08-30  8:22 UTC (permalink / raw)
  To: Ajish Koshy
  Cc: Linux SCSI Mailinglist, Vasanthalakshmi.Tharmarajan, Viswas G,
	Ruksar Devadi, Ashokkumar N, Jinpu Wang

On Mon, Aug 23, 2021 at 9:28 AM Ajish Koshy <Ajish.Koshy@microchip.com> wrote:
>
> Upstream commit <1f02beff224e> introduced a lock per outbound
> queue, where the driver before that was using a global lock
> for all outbound queues. While processing the IO responses
> and events, driver takes the outbound queue spinlock and
> later it is supposed to release the same spin lock in
> pm8001_ccb_task_free_done() before calling command done().
> Since the older code was using a global lock,
> pm8001_ccb_task_free_done() was also releasing the global
> spin lock. With the commit <1f02beff224e>,
> pm8001_ccb_task_free_done() remains the same and it was
> still using the global lock.
>
> So when driver completes a SATA command,the global spinlock
> will be in a locked state.
> mpi_sata_completion()->spin_lock(&pm8001_ha->lock);
>
> Later when driver gets a scsi command for SATA drive,
> pm8001_task_exec() tries to acquire the global lock and leads
> to lockup crash.
>
> Signed-off-by: Ajish Koshy <Ajish.Koshy@microchip.com>
> Signed-off-by: Viswas G <Viswas.G@microchip.com>
The code looks correct, just please refer the commit with standard way eg:
1f02beff224e ("scsi: pm80xx: Remove global lock from outbound queue processing")

Please also add the fixes tag as above, so the stable tree can pick it up.
Thanks!

> ---
>  drivers/scsi/pm8001/pm8001_sas.h |  3 +-
>  drivers/scsi/pm8001/pm80xx_hwi.c | 53 ++++++++++++++++++++++++++------
>  2 files changed, 45 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/scsi/pm8001/pm8001_sas.h b/drivers/scsi/pm8001/pm8001_sas.h
> index 1a016a421280..3274d88a9ccc 100644
> --- a/drivers/scsi/pm8001/pm8001_sas.h
> +++ b/drivers/scsi/pm8001/pm8001_sas.h
> @@ -458,6 +458,7 @@ struct outbound_queue_table {
>         __le32                  producer_index;
>         u32                     consumer_idx;
>         spinlock_t              oq_lock;
> +       unsigned long           lock_flags;
>  };
>  struct pm8001_hba_memspace {
>         void __iomem            *memvirtaddr;
> @@ -740,9 +741,7 @@ pm8001_ccb_task_free_done(struct pm8001_hba_info *pm8001_ha,
>  {
>         pm8001_ccb_task_free(pm8001_ha, task, ccb, ccb_idx);
>         smp_mb(); /*in order to force CPU ordering*/
> -       spin_unlock(&pm8001_ha->lock);
>         task->task_done(task);
> -       spin_lock(&pm8001_ha->lock);
>  }
>
>  #endif
> diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c b/drivers/scsi/pm8001/pm80xx_hwi.c
> index cec932f830b8..1ae2f5c6042c 100644
> --- a/drivers/scsi/pm8001/pm80xx_hwi.c
> +++ b/drivers/scsi/pm8001/pm80xx_hwi.c
> @@ -2379,7 +2379,8 @@ static void mpi_ssp_event(struct pm8001_hba_info *pm8001_ha, void *piomb)
>
>  /*See the comments for mpi_ssp_completion */
>  static void
> -mpi_sata_completion(struct pm8001_hba_info *pm8001_ha, void *piomb)
> +mpi_sata_completion(struct pm8001_hba_info *pm8001_ha,
> +               struct outbound_queue_table *circularQ, void *piomb)
>  {
>         struct sas_task *t;
>         struct pm8001_ccb_info *ccb;
> @@ -2616,7 +2617,11 @@ mpi_sata_completion(struct pm8001_hba_info *pm8001_ha, void *piomb)
>                                 IO_OPEN_CNX_ERROR_IT_NEXUS_LOSS);
>                         ts->resp = SAS_TASK_UNDELIVERED;
>                         ts->stat = SAS_QUEUE_FULL;
> +                       spin_unlock_irqrestore(&circularQ->oq_lock,
> +                                       circularQ->lock_flags);
>                         pm8001_ccb_task_free_done(pm8001_ha, t, ccb, tag);
> +                       spin_lock_irqsave(&circularQ->oq_lock,
> +                                       circularQ->lock_flags);
>                         return;
>                 }
>                 break;
> @@ -2632,7 +2637,11 @@ mpi_sata_completion(struct pm8001_hba_info *pm8001_ha, void *piomb)
>                                 IO_OPEN_CNX_ERROR_IT_NEXUS_LOSS);
>                         ts->resp = SAS_TASK_UNDELIVERED;
>                         ts->stat = SAS_QUEUE_FULL;
> +                       spin_unlock_irqrestore(&circularQ->oq_lock,
> +                                       circularQ->lock_flags);
>                         pm8001_ccb_task_free_done(pm8001_ha, t, ccb, tag);
> +                       spin_lock_irqsave(&circularQ->oq_lock,
> +                                       circularQ->lock_flags);
>                         return;
>                 }
>                 break;
> @@ -2656,7 +2665,11 @@ mpi_sata_completion(struct pm8001_hba_info *pm8001_ha, void *piomb)
>                                 IO_OPEN_CNX_ERROR_STP_RESOURCES_BUSY);
>                         ts->resp = SAS_TASK_UNDELIVERED;
>                         ts->stat = SAS_QUEUE_FULL;
> +                       spin_unlock_irqrestore(&circularQ->oq_lock,
> +                                       circularQ->lock_flags);
>                         pm8001_ccb_task_free_done(pm8001_ha, t, ccb, tag);
> +                       spin_lock_irqsave(&circularQ->oq_lock,
> +                                       circularQ->lock_flags);
>                         return;
>                 }
>                 break;
> @@ -2727,7 +2740,11 @@ mpi_sata_completion(struct pm8001_hba_info *pm8001_ha, void *piomb)
>                                         IO_DS_NON_OPERATIONAL);
>                         ts->resp = SAS_TASK_UNDELIVERED;
>                         ts->stat = SAS_QUEUE_FULL;
> +                       spin_unlock_irqrestore(&circularQ->oq_lock,
> +                                       circularQ->lock_flags);
>                         pm8001_ccb_task_free_done(pm8001_ha, t, ccb, tag);
> +                       spin_lock_irqsave(&circularQ->oq_lock,
> +                                       circularQ->lock_flags);
>                         return;
>                 }
>                 break;
> @@ -2747,7 +2764,11 @@ mpi_sata_completion(struct pm8001_hba_info *pm8001_ha, void *piomb)
>                                         IO_DS_IN_ERROR);
>                         ts->resp = SAS_TASK_UNDELIVERED;
>                         ts->stat = SAS_QUEUE_FULL;
> +                       spin_unlock_irqrestore(&circularQ->oq_lock,
> +                                       circularQ->lock_flags);
>                         pm8001_ccb_task_free_done(pm8001_ha, t, ccb, tag);
> +                       spin_lock_irqsave(&circularQ->oq_lock,
> +                                       circularQ->lock_flags);
>                         return;
>                 }
>                 break;
> @@ -2785,12 +2806,17 @@ mpi_sata_completion(struct pm8001_hba_info *pm8001_ha, void *piomb)
>                 pm8001_ccb_task_free(pm8001_ha, t, ccb, tag);
>         } else {
>                 spin_unlock_irqrestore(&t->task_state_lock, flags);
> +               spin_unlock_irqrestore(&circularQ->oq_lock,
> +                               circularQ->lock_flags);
>                 pm8001_ccb_task_free_done(pm8001_ha, t, ccb, tag);
> +               spin_lock_irqsave(&circularQ->oq_lock,
> +                               circularQ->lock_flags);
>         }
>  }
>
>  /*See the comments for mpi_ssp_completion */
> -static void mpi_sata_event(struct pm8001_hba_info *pm8001_ha, void *piomb)
> +static void mpi_sata_event(struct pm8001_hba_info *pm8001_ha,
> +               struct outbound_queue_table *circularQ, void *piomb)
>  {
>         struct sas_task *t;
>         struct task_status_struct *ts;
> @@ -2890,7 +2916,11 @@ static void mpi_sata_event(struct pm8001_hba_info *pm8001_ha, void *piomb)
>                                 IO_OPEN_CNX_ERROR_IT_NEXUS_LOSS);
>                         ts->resp = SAS_TASK_COMPLETE;
>                         ts->stat = SAS_QUEUE_FULL;
> +                       spin_unlock_irqrestore(&circularQ->oq_lock,
> +                                       circularQ->lock_flags);
>                         pm8001_ccb_task_free_done(pm8001_ha, t, ccb, tag);
> +                       spin_lock_irqsave(&circularQ->oq_lock,
> +                                       circularQ->lock_flags);
>                         return;
>                 }
>                 break;
> @@ -3002,7 +3032,11 @@ static void mpi_sata_event(struct pm8001_hba_info *pm8001_ha, void *piomb)
>                 pm8001_ccb_task_free(pm8001_ha, t, ccb, tag);
>         } else {
>                 spin_unlock_irqrestore(&t->task_state_lock, flags);
> +               spin_unlock_irqrestore(&circularQ->oq_lock,
> +                               circularQ->lock_flags);
>                 pm8001_ccb_task_free_done(pm8001_ha, t, ccb, tag);
> +               spin_lock_irqsave(&circularQ->oq_lock,
> +                               circularQ->lock_flags);
>         }
>  }
>
> @@ -3906,7 +3940,8 @@ static int ssp_coalesced_comp_resp(struct pm8001_hba_info *pm8001_ha,
>   * @pm8001_ha: our hba card information
>   * @piomb: IO message buffer
>   */
> -static void process_one_iomb(struct pm8001_hba_info *pm8001_ha, void *piomb)
> +static void process_one_iomb(struct pm8001_hba_info *pm8001_ha,
> +               struct outbound_queue_table *circularQ, void *piomb)
>  {
>         __le32 pHeader = *(__le32 *)piomb;
>         u32 opc = (u32)((le32_to_cpu(pHeader)) & 0xFFF);
> @@ -3948,11 +3983,11 @@ static void process_one_iomb(struct pm8001_hba_info *pm8001_ha, void *piomb)
>                 break;
>         case OPC_OUB_SATA_COMP:
>                 pm8001_dbg(pm8001_ha, MSG, "OPC_OUB_SATA_COMP\n");
> -               mpi_sata_completion(pm8001_ha, piomb);
> +               mpi_sata_completion(pm8001_ha, circularQ, piomb);
>                 break;
>         case OPC_OUB_SATA_EVENT:
>                 pm8001_dbg(pm8001_ha, MSG, "OPC_OUB_SATA_EVENT\n");
> -               mpi_sata_event(pm8001_ha, piomb);
> +               mpi_sata_event(pm8001_ha, circularQ, piomb);
>                 break;
>         case OPC_OUB_SSP_EVENT:
>                 pm8001_dbg(pm8001_ha, MSG, "OPC_OUB_SSP_EVENT\n");
> @@ -4121,7 +4156,6 @@ static int process_oq(struct pm8001_hba_info *pm8001_ha, u8 vec)
>         void *pMsg1 = NULL;
>         u8 bc;
>         u32 ret = MPI_IO_STATUS_FAIL;
> -       unsigned long flags;
>         u32 regval;
>
>         if (vec == (pm8001_ha->max_q_num - 1)) {
> @@ -4138,7 +4172,7 @@ static int process_oq(struct pm8001_hba_info *pm8001_ha, u8 vec)
>                 }
>         }
>         circularQ = &pm8001_ha->outbnd_q_tbl[vec];
> -       spin_lock_irqsave(&circularQ->oq_lock, flags);
> +       spin_lock_irqsave(&circularQ->oq_lock, circularQ->lock_flags);
>         do {
>                 /* spurious interrupt during setup if kexec-ing and
>                  * driver doing a doorbell access w/ the pre-kexec oq
> @@ -4149,7 +4183,8 @@ static int process_oq(struct pm8001_hba_info *pm8001_ha, u8 vec)
>                 ret = pm8001_mpi_msg_consume(pm8001_ha, circularQ, &pMsg1, &bc);
>                 if (MPI_IO_STATUS_SUCCESS == ret) {
>                         /* process the outbound message */
> -                       process_one_iomb(pm8001_ha, (void *)(pMsg1 - 4));
> +                       process_one_iomb(pm8001_ha, circularQ,
> +                                               (void *)(pMsg1 - 4));
>                         /* free the message from the outbound circular buffer */
>                         pm8001_mpi_msg_free_set(pm8001_ha, pMsg1,
>                                                         circularQ, bc);
> @@ -4164,7 +4199,7 @@ static int process_oq(struct pm8001_hba_info *pm8001_ha, u8 vec)
>                                 break;
>                 }
>         } while (1);
> -       spin_unlock_irqrestore(&circularQ->oq_lock, flags);
> +       spin_unlock_irqrestore(&circularQ->oq_lock, circularQ->lock_flags);
>         return ret;
>  }
>
> --
> 2.27.0
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 3/4] scsi: pm80xx: Corrected Inbound and Outbound queue logging
  2021-08-23  8:23 ` [PATCH 3/4] scsi: pm80xx: Corrected Inbound and Outbound queue logging Ajish Koshy
@ 2021-08-30  8:23   ` Jinpu Wang
  0 siblings, 0 replies; 11+ messages in thread
From: Jinpu Wang @ 2021-08-30  8:23 UTC (permalink / raw)
  To: Ajish Koshy
  Cc: Linux SCSI Mailinglist, Vasanthalakshmi.Tharmarajan, Viswas G,
	Ruksar Devadi, Ashokkumar N, Jinpu Wang

On Mon, Aug 23, 2021 at 9:28 AM Ajish Koshy <Ajish.Koshy@microchip.com> wrote:
>
> From: Viswas G <Viswas.G@microchip.com>
>
> Corrected inbound queue and outbound queue size in 'ib_log'
> and 'ob_log' sysfs entries.
>
> Signed-off-by: Viswas G <Viswas.G@microchip.com>
> Signed-off-by: Ajish Koshy <Ajish.Koshy@microchip.com>
Acked-by: Jack Wang <jinpu.wang@ionos.com>
Thanks
> ---
>  drivers/scsi/pm8001/pm8001_ctl.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/pm8001/pm8001_ctl.c b/drivers/scsi/pm8001/pm8001_ctl.c
> index ec05c42e8ee6..b25e447aa3bd 100644
> --- a/drivers/scsi/pm8001/pm8001_ctl.c
> +++ b/drivers/scsi/pm8001/pm8001_ctl.c
> @@ -409,6 +409,7 @@ static ssize_t pm8001_ctl_ib_queue_log_show(struct device *cdev,
>         char *str = buf;
>         int start = 0;
>         u32 ib_offset = pm8001_ha->ib_offset;
> +       u32 queue_size = pm8001_ha->max_q_num * PM8001_MPI_QUEUE * 128;
>  #define IB_MEMMAP(c)   \
>                 (*(u32 *)((u8 *)pm8001_ha->     \
>                 memoryMap.region[ib_offset].virt_ptr +  \
> @@ -419,7 +420,7 @@ static ssize_t pm8001_ctl_ib_queue_log_show(struct device *cdev,
>                 start = start + 4;
>         }
>         pm8001_ha->evtlog_ib_offset += SYSFS_OFFSET;
> -       if (((pm8001_ha->evtlog_ib_offset) % (PM80XX_IB_OB_QUEUE_SIZE)) == 0)
> +       if (((pm8001_ha->evtlog_ib_offset) % queue_size) == 0)
>                 pm8001_ha->evtlog_ib_offset = 0;
>
>         return str - buf;
> @@ -445,6 +446,7 @@ static ssize_t pm8001_ctl_ob_queue_log_show(struct device *cdev,
>         char *str = buf;
>         int start = 0;
>         u32 ob_offset = pm8001_ha->ob_offset;
> +       u32 queue_size = pm8001_ha->max_q_num * PM8001_MPI_QUEUE * 128;
>  #define OB_MEMMAP(c)   \
>                 (*(u32 *)((u8 *)pm8001_ha->     \
>                 memoryMap.region[ob_offset].virt_ptr +  \
> @@ -455,7 +457,7 @@ static ssize_t pm8001_ctl_ob_queue_log_show(struct device *cdev,
>                 start = start + 4;
>         }
>         pm8001_ha->evtlog_ob_offset += SYSFS_OFFSET;
> -       if (((pm8001_ha->evtlog_ob_offset) % (PM80XX_IB_OB_QUEUE_SIZE)) == 0)
> +       if (((pm8001_ha->evtlog_ob_offset) % queue_size) == 0)
>                 pm8001_ha->evtlog_ob_offset = 0;
>
>         return str - buf;
> --
> 2.27.0
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 4/4] scsi: pm80xx: fix memory leak during rmmod
  2021-08-23  8:23 ` [PATCH 4/4] scsi: pm80xx: fix memory leak during rmmod Ajish Koshy
@ 2021-08-30  8:25   ` Jinpu Wang
  0 siblings, 0 replies; 11+ messages in thread
From: Jinpu Wang @ 2021-08-30  8:25 UTC (permalink / raw)
  To: Ajish Koshy
  Cc: Linux SCSI Mailinglist, Vasanthalakshmi.Tharmarajan, Viswas G,
	Ruksar Devadi, Ashokkumar N, Jinpu Wang

On Mon, Aug 23, 2021 at 9:28 AM Ajish Koshy <Ajish.Koshy@microchip.com> wrote:
>
> Driver fails to release memory allocated. This will lead
> to memory leak during driver removal.
>
> Signed-off-by: Ajish Koshy <Ajish.Koshy@microchip.com>
> Signed-off-by: Viswas G <Viswas.G@microchip.com>
looks ok.
Acked-by: Jack Wang <jinpu.wang@ionos.com>
> ---
>  drivers/scsi/pm8001/pm8001_init.c | 11 +++++++++++
>  drivers/scsi/pm8001/pm8001_sas.h  |  1 +
>  2 files changed, 12 insertions(+)
>
> diff --git a/drivers/scsi/pm8001/pm8001_init.c b/drivers/scsi/pm8001/pm8001_init.c
> index 613455a3e686..7082fecf7ce8 100644
> --- a/drivers/scsi/pm8001/pm8001_init.c
> +++ b/drivers/scsi/pm8001/pm8001_init.c
> @@ -1199,6 +1199,7 @@ pm8001_init_ccb_tag(struct pm8001_hba_info *pm8001_ha, struct Scsi_Host *shost,
>                 goto err_out;
>
>         /* Memory region for ccb_info*/
> +       pm8001_ha->ccb_count = ccb_count;
>         pm8001_ha->ccb_info =
>                 kcalloc(ccb_count, sizeof(struct pm8001_ccb_info), GFP_KERNEL);
>         if (!pm8001_ha->ccb_info) {
> @@ -1260,6 +1261,16 @@ static void pm8001_pci_remove(struct pci_dev *pdev)
>                         tasklet_kill(&pm8001_ha->tasklet[j]);
>  #endif
>         scsi_host_put(pm8001_ha->shost);
> +
> +       for (i = 0; i < pm8001_ha->ccb_count; i++) {
> +               dma_free_coherent(&pm8001_ha->pdev->dev,
> +                       sizeof(struct pm8001_prd) * PM8001_MAX_DMA_SG,
> +                       pm8001_ha->ccb_info[i].buf_prd,
> +                       pm8001_ha->ccb_info[i].ccb_dma_handle);
> +       }
> +       kfree(pm8001_ha->ccb_info);
> +       kfree(pm8001_ha->devices);
> +
>         pm8001_free(pm8001_ha);
>         kfree(sha->sas_phy);
>         kfree(sha->sas_port);
> diff --git a/drivers/scsi/pm8001/pm8001_sas.h b/drivers/scsi/pm8001/pm8001_sas.h
> index 3274d88a9ccc..7e999768bfd2 100644
> --- a/drivers/scsi/pm8001/pm8001_sas.h
> +++ b/drivers/scsi/pm8001/pm8001_sas.h
> @@ -518,6 +518,7 @@ struct pm8001_hba_info {
>         u32                     iomb_size; /* SPC and SPCV IOMB size */
>         struct pm8001_device    *devices;
>         struct pm8001_ccb_info  *ccb_info;
> +       u32                     ccb_count;
>  #ifdef PM8001_USE_MSIX
>         int                     number_of_intr;/*will be used in remove()*/
>         char                    intr_drvname[PM8001_MAX_MSIX_VEC]
> --
> 2.27.0
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH 1/4] scsi: pm80xx: fix incorrect port value when registering a device
  2021-08-30  7:58   ` Jinpu Wang
@ 2021-09-01 11:44     ` Ajish.Koshy
  0 siblings, 0 replies; 11+ messages in thread
From: Ajish.Koshy @ 2021-09-01 11:44 UTC (permalink / raw)
  To: jinpu.wang
  Cc: linux-scsi, Vasanthalakshmi.Tharmarajan, Viswas.G, Ruksar.devadi,
	Ashokkumar.N, jinpu.wang


Hi Jinpu,

Thanks for your review.

During "phy up" event, firmware gives the phy_id and port_id 
and driver is supposed to use the same during device handle 
registration. Earlier, driver was using port id value from libsas
during device handle registration and at times, it is different from
firmware assigned port id. This will lead to wrong device registration 
and eventually we won’t see those drives.

We will add these details in V2 patch set commit message.

Thanks,
Ajish
-----Original Message-----
From: Jinpu Wang <jinpu.wang@ionos.com> 
Sent: Monday, August 30, 2021 01:29 PM
To: Ajish Koshy - I30923 <Ajish.Koshy@microchip.com>
Cc: Linux SCSI Mailinglist <linux-scsi@vger.kernel.org>; Vasanthalakshmi Tharmarajan - I30664 <Vasanthalakshmi.Tharmarajan@microchip.com>; Viswas G - I30667 <Viswas.G@microchip.com>; Ruksar Devadi - I52327 <Ruksar.devadi@microchip.com>; Ashokkumar N - X53535 <Ashokkumar.N@microchip.com>; Jinpu Wang <jinpu.wang@cloud.ionos.com>
Subject: Re: [PATCH 1/4] scsi: pm80xx: fix incorrect port value when registering a device

[You don't often get email from jinpu.wang@ionos.com. Learn why this is important at http://aka.ms/LearnAboutSenderIdentification.]

EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe

Hi Ajish,
On Mon, Aug 23, 2021 at 9:28 AM Ajish Koshy <Ajish.Koshy@microchip.com> wrote:
>
> Sometime we observe there is mismatch between portid & phyid received 
> during phyup event and device registration. Later this will cause 
> drive missing issue.
Thanks for the patch.
Could you explain a bit more, what was the problem, and how you fix it in the commit message?
>
> Signed-off-by: Ajish Koshy <Ajish.Koshy@microchip.com>
> Signed-off-by: Viswas G <Viswas.G@microchip.com>
> ---
>  drivers/scsi/pm8001/pm8001_hwi.c  |  7 ++++++-  
> drivers/scsi/pm8001/pm8001_init.c |  1 +  
> drivers/scsi/pm8001/pm8001_sas.c  | 15 +++++++++++++++  
> drivers/scsi/pm8001/pm8001_sas.h  |  2 ++  
> drivers/scsi/pm8001/pm80xx_hwi.c  |  7 ++++++-
>  5 files changed, 30 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/pm8001/pm8001_hwi.c 
> b/drivers/scsi/pm8001/pm8001_hwi.c
> index 63690508313b..c9ecddd0d719 100644
> --- a/drivers/scsi/pm8001/pm8001_hwi.c
> +++ b/drivers/scsi/pm8001/pm8001_hwi.c
> @@ -3358,6 +3358,8 @@ hw_event_sas_phy_up(struct pm8001_hba_info *pm8001_ha, void *piomb)
>         struct pm8001_phy *phy = &pm8001_ha->phy[phy_id];
>         unsigned long flags;
>         u8 deviceType = pPayload->sas_identify.dev_type;
> +       phy->port = port;
> +       port->port_id = port_id;
>         port->port_state =  portstate;
>         phy->phy_state = PHY_STATE_LINK_UP_SPC;
>         pm8001_dbg(pm8001_ha, MSG,
> @@ -3434,6 +3436,8 @@ hw_event_sata_phy_up(struct pm8001_hba_info *pm8001_ha, void *piomb)
>         unsigned long flags;
>         pm8001_dbg(pm8001_ha, DEVIO, "HW_EVENT_SATA_PHY_UP port id = %d, phy id = %d\n",
>                    port_id, phy_id);
> +       phy->port = port;
> +       port->port_id = port_id;
>         port->port_state =  portstate;
>         phy->phy_state = PHY_STATE_LINK_UP_SPC;
>         port->port_attached = 1;
> @@ -4460,6 +4464,7 @@ static int pm8001_chip_reg_dev_req(struct pm8001_hba_info *pm8001_ha,
>         u16 ITNT = 2000;
>         struct domain_device *dev = pm8001_dev->sas_device;
>         struct domain_device *parent_dev = dev->parent;
> +       struct pm8001_port *port = dev->port->lldd_port;
>         circularQ = &pm8001_ha->inbnd_q_tbl[0];
>
>         memset(&payload, 0, sizeof(payload)); @@ -4488,7 +4493,7 @@ 
> static int pm8001_chip_reg_dev_req(struct pm8001_hba_info *pm8001_ha,
>         linkrate = (pm8001_dev->sas_device->linkrate < dev->port->linkrate) ?
>                         pm8001_dev->sas_device->linkrate : dev->port->linkrate;
>         payload.phyid_portid =
> -               cpu_to_le32(((pm8001_dev->sas_device->port->id) & 0x0F) |
> +               cpu_to_le32(((port->port_id) & 0x0F) |
>                 ((phy_id & 0x0F) << 4));
>         payload.dtype_dlr_retry = cpu_to_le32((retryFlag & 0x01) |
>                 ((linkrate & 0x0F) * 0x1000000) | diff --git 
> a/drivers/scsi/pm8001/pm8001_init.c 
> b/drivers/scsi/pm8001/pm8001_init.c
> index 47db7e0beae6..613455a3e686 100644
> --- a/drivers/scsi/pm8001/pm8001_init.c
> +++ b/drivers/scsi/pm8001/pm8001_init.c
> @@ -128,6 +128,7 @@ static struct sas_domain_function_template pm8001_transport_ops = {
>         .lldd_I_T_nexus_reset   = pm8001_I_T_nexus_reset,
>         .lldd_lu_reset          = pm8001_lu_reset,
>         .lldd_query_task        = pm8001_query_task,
> +       .lldd_port_formed       = pm8001_port_formed,
>  };
>
>  /**
> diff --git a/drivers/scsi/pm8001/pm8001_sas.c 
> b/drivers/scsi/pm8001/pm8001_sas.c
> index 32e60f0c3b14..83e73009db5c 100644
> --- a/drivers/scsi/pm8001/pm8001_sas.c
> +++ b/drivers/scsi/pm8001/pm8001_sas.c
> @@ -1355,3 +1355,18 @@ int pm8001_clear_task_set(struct domain_device *dev, u8 *lun)
>         tmf_task.tmf = TMF_CLEAR_TASK_SET;
>         return pm8001_issue_ssp_tmf(dev, lun, &tmf_task);  }
> +
> +void pm8001_port_formed(struct asd_sas_phy *sas_phy) {
> +       struct sas_ha_struct *sas_ha = sas_phy->ha;
> +       struct pm8001_hba_info *pm8001_ha = sas_ha->lldd_ha;
> +       struct pm8001_phy *phy = sas_phy->lldd_phy;
> +       struct asd_sas_port *sas_port = sas_phy->port;
> +       struct pm8001_port *port = phy->port;
> +
> +       if (!sas_port) {
> +               pm8001_dbg(pm8001_ha, FAIL, "Received null port\n");
> +               return;
> +       }
> +       sas_port->lldd_port = port;
> +}
> diff --git a/drivers/scsi/pm8001/pm8001_sas.h 
> b/drivers/scsi/pm8001/pm8001_sas.h
> index 62d08b535a4b..1a016a421280 100644
> --- a/drivers/scsi/pm8001/pm8001_sas.h
> +++ b/drivers/scsi/pm8001/pm8001_sas.h
> @@ -230,6 +230,7 @@ struct pm8001_port {
>         u8                      port_attached;
>         u16                     wide_port_phymap;
>         u8                      port_state;
> +       u8                      port_id;
>         struct list_head        list;
>  };
>
> @@ -651,6 +652,7 @@ int pm8001_lu_reset(struct domain_device *dev, u8 
> *lun);  int pm8001_I_T_nexus_reset(struct domain_device *dev);  int 
> pm8001_I_T_nexus_event_handler(struct domain_device *dev);  int 
> pm8001_query_task(struct sas_task *task);
> +void pm8001_port_formed(struct asd_sas_phy *sas_phy);
>  void pm8001_open_reject_retry(
>         struct pm8001_hba_info *pm8001_ha,
>         struct sas_task *task_to_close, diff --git 
> a/drivers/scsi/pm8001/pm80xx_hwi.c b/drivers/scsi/pm8001/pm80xx_hwi.c
> index 6ffe17b849ae..cec932f830b8 100644
> --- a/drivers/scsi/pm8001/pm80xx_hwi.c
> +++ b/drivers/scsi/pm8001/pm80xx_hwi.c
> @@ -3299,6 +3299,8 @@ hw_event_sas_phy_up(struct pm8001_hba_info *pm8001_ha, void *piomb)
>         struct pm8001_phy *phy = &pm8001_ha->phy[phy_id];
>         unsigned long flags;
>         u8 deviceType = pPayload->sas_identify.dev_type;
> +       phy->port = port;
> +       port->port_id = port_id;
>         port->port_state = portstate;
>         port->wide_port_phymap |= (1U << phy_id);
>         phy->phy_state = PHY_STATE_LINK_UP_SPCV; @@ -3380,6 +3382,8 @@ 
> hw_event_sata_phy_up(struct pm8001_hba_info *pm8001_ha, void *piomb)
>                    "port id %d, phy id %d link_rate %d portstate 0x%x\n",
>                    port_id, phy_id, link_rate, portstate);
>
> +       phy->port = port;
> +       port->port_id = port_id;
>         port->port_state = portstate;
>         phy->phy_state = PHY_STATE_LINK_UP_SPCV;
>         port->port_attached = 1;
> @@ -4808,6 +4812,7 @@ static int pm80xx_chip_reg_dev_req(struct pm8001_hba_info *pm8001_ha,
>         u16 ITNT = 2000;
>         struct domain_device *dev = pm8001_dev->sas_device;
>         struct domain_device *parent_dev = dev->parent;
> +       struct pm8001_port *port = dev->port->lldd_port;
>         circularQ = &pm8001_ha->inbnd_q_tbl[0];
>
>         memset(&payload, 0, sizeof(payload)); @@ -4840,7 +4845,7 @@ 
> static int pm80xx_chip_reg_dev_req(struct pm8001_hba_info *pm8001_ha,
>                         pm8001_dev->sas_device->linkrate : 
> dev->port->linkrate;
>
>         payload.phyid_portid =
> -               cpu_to_le32(((pm8001_dev->sas_device->port->id) & 0xFF) |
> +               cpu_to_le32(((port->port_id) & 0xFF) |
>                 ((phy_id & 0xFF) << 8));
>
>         payload.dtype_dlr_mcn_ir_retry = cpu_to_le32((retryFlag & 
> 0x01) |
> --
> 2.27.0
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH 2/4] scsi: pm80xx: fix lockup due to commit <1f02beff224e>
  2021-08-30  8:22   ` Jinpu Wang
@ 2021-09-01 11:55     ` Ajish.Koshy
  0 siblings, 0 replies; 11+ messages in thread
From: Ajish.Koshy @ 2021-09-01 11:55 UTC (permalink / raw)
  To: jinpu.wang
  Cc: linux-scsi, Vasanthalakshmi.Tharmarajan, Viswas.G, Ruksar.devadi,
	Ashokkumar.N, jinpu.wang

Hi Jinpu,

Thanks for the review.

We will make changes accordingly in V2 patch set commit message.

Thanks,
Ajish

-----Original Message-----
From: Jinpu Wang <jinpu.wang@ionos.com> 
Sent: Monday, August 30, 2021 01:52 PM
To: Ajish Koshy - I30923 <Ajish.Koshy@microchip.com>
Cc: Linux SCSI Mailinglist <linux-scsi@vger.kernel.org>; Vasanthalakshmi Tharmarajan - I30664 <Vasanthalakshmi.Tharmarajan@microchip.com>; Viswas G - I30667 <Viswas.G@microchip.com>; Ruksar Devadi - I52327 <Ruksar.devadi@microchip.com>; Ashokkumar N - X53535 <Ashokkumar.N@microchip.com>; Jinpu Wang <jinpu.wang@cloud.ionos.com>
Subject: Re: [PATCH 2/4] scsi: pm80xx: fix lockup due to commit <1f02beff224e>

[You don't often get email from jinpu.wang@ionos.com. Learn why this is important at http://aka.ms/LearnAboutSenderIdentification.]

EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe

On Mon, Aug 23, 2021 at 9:28 AM Ajish Koshy <Ajish.Koshy@microchip.com> wrote:
>
> Upstream commit <1f02beff224e> introduced a lock per outbound queue, 
> where the driver before that was using a global lock for all outbound 
> queues. While processing the IO responses and events, driver takes the 
> outbound queue spinlock and later it is supposed to release the same 
> spin lock in
> pm8001_ccb_task_free_done() before calling command done().
> Since the older code was using a global lock,
> pm8001_ccb_task_free_done() was also releasing the global spin lock. 
> With the commit <1f02beff224e>,
> pm8001_ccb_task_free_done() remains the same and it was still using 
> the global lock.
>
> So when driver completes a SATA command,the global spinlock will be in 
> a locked state.
> mpi_sata_completion()->spin_lock(&pm8001_ha->lock);
>
> Later when driver gets a scsi command for SATA drive,
> pm8001_task_exec() tries to acquire the global lock and leads to 
> lockup crash.
>
> Signed-off-by: Ajish Koshy <Ajish.Koshy@microchip.com>
> Signed-off-by: Viswas G <Viswas.G@microchip.com>
The code looks correct, just please refer the commit with standard way eg:
1f02beff224e ("scsi: pm80xx: Remove global lock from outbound queue processing")

Please also add the fixes tag as above, so the stable tree can pick it up.
Thanks!

> ---
>  drivers/scsi/pm8001/pm8001_sas.h |  3 +-  
> drivers/scsi/pm8001/pm80xx_hwi.c | 53 ++++++++++++++++++++++++++------
>  2 files changed, 45 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/scsi/pm8001/pm8001_sas.h 
> b/drivers/scsi/pm8001/pm8001_sas.h
> index 1a016a421280..3274d88a9ccc 100644
> --- a/drivers/scsi/pm8001/pm8001_sas.h
> +++ b/drivers/scsi/pm8001/pm8001_sas.h
> @@ -458,6 +458,7 @@ struct outbound_queue_table {
>         __le32                  producer_index;
>         u32                     consumer_idx;
>         spinlock_t              oq_lock;
> +       unsigned long           lock_flags;
>  };
>  struct pm8001_hba_memspace {
>         void __iomem            *memvirtaddr;
> @@ -740,9 +741,7 @@ pm8001_ccb_task_free_done(struct pm8001_hba_info 
> *pm8001_ha,  {
>         pm8001_ccb_task_free(pm8001_ha, task, ccb, ccb_idx);
>         smp_mb(); /*in order to force CPU ordering*/
> -       spin_unlock(&pm8001_ha->lock);
>         task->task_done(task);
> -       spin_lock(&pm8001_ha->lock);
>  }
>
>  #endif
> diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c 
> b/drivers/scsi/pm8001/pm80xx_hwi.c
> index cec932f830b8..1ae2f5c6042c 100644
> --- a/drivers/scsi/pm8001/pm80xx_hwi.c
> +++ b/drivers/scsi/pm8001/pm80xx_hwi.c
> @@ -2379,7 +2379,8 @@ static void mpi_ssp_event(struct pm8001_hba_info 
> *pm8001_ha, void *piomb)
>
>  /*See the comments for mpi_ssp_completion */  static void 
> -mpi_sata_completion(struct pm8001_hba_info *pm8001_ha, void *piomb)
> +mpi_sata_completion(struct pm8001_hba_info *pm8001_ha,
> +               struct outbound_queue_table *circularQ, void *piomb)
>  {
>         struct sas_task *t;
>         struct pm8001_ccb_info *ccb;
> @@ -2616,7 +2617,11 @@ mpi_sata_completion(struct pm8001_hba_info *pm8001_ha, void *piomb)
>                                 IO_OPEN_CNX_ERROR_IT_NEXUS_LOSS);
>                         ts->resp = SAS_TASK_UNDELIVERED;
>                         ts->stat = SAS_QUEUE_FULL;
> +                       spin_unlock_irqrestore(&circularQ->oq_lock,
> +                                       circularQ->lock_flags);
>                         pm8001_ccb_task_free_done(pm8001_ha, t, ccb, 
> tag);
> +                       spin_lock_irqsave(&circularQ->oq_lock,
> +                                       circularQ->lock_flags);
>                         return;
>                 }
>                 break;
> @@ -2632,7 +2637,11 @@ mpi_sata_completion(struct pm8001_hba_info *pm8001_ha, void *piomb)
>                                 IO_OPEN_CNX_ERROR_IT_NEXUS_LOSS);
>                         ts->resp = SAS_TASK_UNDELIVERED;
>                         ts->stat = SAS_QUEUE_FULL;
> +                       spin_unlock_irqrestore(&circularQ->oq_lock,
> +                                       circularQ->lock_flags);
>                         pm8001_ccb_task_free_done(pm8001_ha, t, ccb, 
> tag);
> +                       spin_lock_irqsave(&circularQ->oq_lock,
> +                                       circularQ->lock_flags);
>                         return;
>                 }
>                 break;
> @@ -2656,7 +2665,11 @@ mpi_sata_completion(struct pm8001_hba_info *pm8001_ha, void *piomb)
>                                 IO_OPEN_CNX_ERROR_STP_RESOURCES_BUSY);
>                         ts->resp = SAS_TASK_UNDELIVERED;
>                         ts->stat = SAS_QUEUE_FULL;
> +                       spin_unlock_irqrestore(&circularQ->oq_lock,
> +                                       circularQ->lock_flags);
>                         pm8001_ccb_task_free_done(pm8001_ha, t, ccb, 
> tag);
> +                       spin_lock_irqsave(&circularQ->oq_lock,
> +                                       circularQ->lock_flags);
>                         return;
>                 }
>                 break;
> @@ -2727,7 +2740,11 @@ mpi_sata_completion(struct pm8001_hba_info *pm8001_ha, void *piomb)
>                                         IO_DS_NON_OPERATIONAL);
>                         ts->resp = SAS_TASK_UNDELIVERED;
>                         ts->stat = SAS_QUEUE_FULL;
> +                       spin_unlock_irqrestore(&circularQ->oq_lock,
> +                                       circularQ->lock_flags);
>                         pm8001_ccb_task_free_done(pm8001_ha, t, ccb, 
> tag);
> +                       spin_lock_irqsave(&circularQ->oq_lock,
> +                                       circularQ->lock_flags);
>                         return;
>                 }
>                 break;
> @@ -2747,7 +2764,11 @@ mpi_sata_completion(struct pm8001_hba_info *pm8001_ha, void *piomb)
>                                         IO_DS_IN_ERROR);
>                         ts->resp = SAS_TASK_UNDELIVERED;
>                         ts->stat = SAS_QUEUE_FULL;
> +                       spin_unlock_irqrestore(&circularQ->oq_lock,
> +                                       circularQ->lock_flags);
>                         pm8001_ccb_task_free_done(pm8001_ha, t, ccb, 
> tag);
> +                       spin_lock_irqsave(&circularQ->oq_lock,
> +                                       circularQ->lock_flags);
>                         return;
>                 }
>                 break;
> @@ -2785,12 +2806,17 @@ mpi_sata_completion(struct pm8001_hba_info *pm8001_ha, void *piomb)
>                 pm8001_ccb_task_free(pm8001_ha, t, ccb, tag);
>         } else {
>                 spin_unlock_irqrestore(&t->task_state_lock, flags);
> +               spin_unlock_irqrestore(&circularQ->oq_lock,
> +                               circularQ->lock_flags);
>                 pm8001_ccb_task_free_done(pm8001_ha, t, ccb, tag);
> +               spin_lock_irqsave(&circularQ->oq_lock,
> +                               circularQ->lock_flags);
>         }
>  }
>
>  /*See the comments for mpi_ssp_completion */ -static void 
> mpi_sata_event(struct pm8001_hba_info *pm8001_ha, void *piomb)
> +static void mpi_sata_event(struct pm8001_hba_info *pm8001_ha,
> +               struct outbound_queue_table *circularQ, void *piomb)
>  {
>         struct sas_task *t;
>         struct task_status_struct *ts; @@ -2890,7 +2916,11 @@ static 
> void mpi_sata_event(struct pm8001_hba_info *pm8001_ha, void *piomb)
>                                 IO_OPEN_CNX_ERROR_IT_NEXUS_LOSS);
>                         ts->resp = SAS_TASK_COMPLETE;
>                         ts->stat = SAS_QUEUE_FULL;
> +                       spin_unlock_irqrestore(&circularQ->oq_lock,
> +                                       circularQ->lock_flags);
>                         pm8001_ccb_task_free_done(pm8001_ha, t, ccb, 
> tag);
> +                       spin_lock_irqsave(&circularQ->oq_lock,
> +                                       circularQ->lock_flags);
>                         return;
>                 }
>                 break;
> @@ -3002,7 +3032,11 @@ static void mpi_sata_event(struct pm8001_hba_info *pm8001_ha, void *piomb)
>                 pm8001_ccb_task_free(pm8001_ha, t, ccb, tag);
>         } else {
>                 spin_unlock_irqrestore(&t->task_state_lock, flags);
> +               spin_unlock_irqrestore(&circularQ->oq_lock,
> +                               circularQ->lock_flags);
>                 pm8001_ccb_task_free_done(pm8001_ha, t, ccb, tag);
> +               spin_lock_irqsave(&circularQ->oq_lock,
> +                               circularQ->lock_flags);
>         }
>  }
>
> @@ -3906,7 +3940,8 @@ static int ssp_coalesced_comp_resp(struct pm8001_hba_info *pm8001_ha,
>   * @pm8001_ha: our hba card information
>   * @piomb: IO message buffer
>   */
> -static void process_one_iomb(struct pm8001_hba_info *pm8001_ha, void 
> *piomb)
> +static void process_one_iomb(struct pm8001_hba_info *pm8001_ha,
> +               struct outbound_queue_table *circularQ, void *piomb)
>  {
>         __le32 pHeader = *(__le32 *)piomb;
>         u32 opc = (u32)((le32_to_cpu(pHeader)) & 0xFFF); @@ -3948,11 
> +3983,11 @@ static void process_one_iomb(struct pm8001_hba_info *pm8001_ha, void *piomb)
>                 break;
>         case OPC_OUB_SATA_COMP:
>                 pm8001_dbg(pm8001_ha, MSG, "OPC_OUB_SATA_COMP\n");
> -               mpi_sata_completion(pm8001_ha, piomb);
> +               mpi_sata_completion(pm8001_ha, circularQ, piomb);
>                 break;
>         case OPC_OUB_SATA_EVENT:
>                 pm8001_dbg(pm8001_ha, MSG, "OPC_OUB_SATA_EVENT\n");
> -               mpi_sata_event(pm8001_ha, piomb);
> +               mpi_sata_event(pm8001_ha, circularQ, piomb);
>                 break;
>         case OPC_OUB_SSP_EVENT:
>                 pm8001_dbg(pm8001_ha, MSG, "OPC_OUB_SSP_EVENT\n"); @@ 
> -4121,7 +4156,6 @@ static int process_oq(struct pm8001_hba_info *pm8001_ha, u8 vec)
>         void *pMsg1 = NULL;
>         u8 bc;
>         u32 ret = MPI_IO_STATUS_FAIL;
> -       unsigned long flags;
>         u32 regval;
>
>         if (vec == (pm8001_ha->max_q_num - 1)) { @@ -4138,7 +4172,7 @@ 
> static int process_oq(struct pm8001_hba_info *pm8001_ha, u8 vec)
>                 }
>         }
>         circularQ = &pm8001_ha->outbnd_q_tbl[vec];
> -       spin_lock_irqsave(&circularQ->oq_lock, flags);
> +       spin_lock_irqsave(&circularQ->oq_lock, circularQ->lock_flags);
>         do {
>                 /* spurious interrupt during setup if kexec-ing and
>                  * driver doing a doorbell access w/ the pre-kexec oq 
> @@ -4149,7 +4183,8 @@ static int process_oq(struct pm8001_hba_info *pm8001_ha, u8 vec)
>                 ret = pm8001_mpi_msg_consume(pm8001_ha, circularQ, &pMsg1, &bc);
>                 if (MPI_IO_STATUS_SUCCESS == ret) {
>                         /* process the outbound message */
> -                       process_one_iomb(pm8001_ha, (void *)(pMsg1 - 4));
> +                       process_one_iomb(pm8001_ha, circularQ,
> +                                               (void *)(pMsg1 - 4));
>                         /* free the message from the outbound circular buffer */
>                         pm8001_mpi_msg_free_set(pm8001_ha, pMsg1,
>                                                         circularQ, 
> bc); @@ -4164,7 +4199,7 @@ static int process_oq(struct pm8001_hba_info *pm8001_ha, u8 vec)
>                                 break;
>                 }
>         } while (1);
> -       spin_unlock_irqrestore(&circularQ->oq_lock, flags);
> +       spin_unlock_irqrestore(&circularQ->oq_lock, 
> + circularQ->lock_flags);
>         return ret;
>  }
>
> --
> 2.27.0
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2021-09-01 11:55 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-23  8:23 [PATCH 0/4] pm80xx updates Ajish Koshy
2021-08-23  8:23 ` [PATCH 1/4] scsi: pm80xx: fix incorrect port value when registering a device Ajish Koshy
2021-08-30  7:58   ` Jinpu Wang
2021-09-01 11:44     ` Ajish.Koshy
2021-08-23  8:23 ` [PATCH 2/4] scsi: pm80xx: fix lockup due to commit <1f02beff224e> Ajish Koshy
2021-08-30  8:22   ` Jinpu Wang
2021-09-01 11:55     ` Ajish.Koshy
2021-08-23  8:23 ` [PATCH 3/4] scsi: pm80xx: Corrected Inbound and Outbound queue logging Ajish Koshy
2021-08-30  8:23   ` Jinpu Wang
2021-08-23  8:23 ` [PATCH 4/4] scsi: pm80xx: fix memory leak during rmmod Ajish Koshy
2021-08-30  8:25   ` Jinpu Wang

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.