linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4 00/11] Enhance libsas hotplug feature
@ 2017-09-06  9:15 Jason Yan
  2017-09-06  9:15 ` [PATCH v4 01/11] libsas: kill useless ha_event and do some cleanup Jason Yan
                   ` (13 more replies)
  0 siblings, 14 replies; 28+ messages in thread
From: Jason Yan @ 2017-09-06  9:15 UTC (permalink / raw)
  To: martin.petersen, jejb
  Cc: linux-scsi, linux-kernel, john.garry, chenweilong, zhaohongjiang,
	hare, dan.j.williams, jthumshirn, Raj.Dinesh, hch, huangdaode,
	chenxiang66, yanaijie

Hello all, Yijing Wang handed over this topic to me. We are working
on it the last two months. We have tested the patchset for a long
time. Here is the new version.

Now the libsas hotplug has some issues, Dan Williams report
a similar bug here before
https://www.mail-archive.com/linux-scsi@vger.kernel.org/msg39187.html

The issues we have found
1. if LLDD burst reports lots of phy-up/phy-down sas events, some events
   may lost because a same sas events is pending now, finally libsas topo
   may different the hardware.
2. receive a phy down sas event, libsas call sas_deform_port to remove
   devices, it would first delete the sas port, then put a destruction
   discovery event in a new work, and queue it at the tail of workqueue,
   once the sas port be deleted, its children device will be deleted too,
   when the destruction work start, it will found the target device has
   been removed, and report a sysfs warnning.
3. since a hotplug process will be divided into several works, if a phy up
   sas event insert into phydown works, like
   destruction work  ---> PORTE_BYTES_DMAED (sas_form_port) ---->PHYE_LOSS_OF_SIGNAL
   the hot remove flow would broken by PORTE_BYTES_DMAED event, it's not
   we expected, and issues would occur.

v3->v4: -get rid of unused ha event and do some cleanup
        -use dynamic alloced work and support shutting down the phy if active event reached the threshold
		-use flush_workqueue instead of wait-completion to process discover events synchronously
		-direct call probe and destruct function
		-other small code improvements 
v2->v3: some code improvements suggested by Johannes and John,
	    split v2 patch 2 into several small patches.
v1->v2: some code improvements suggested by John Garry

Jason Yan (10):
  libsas: kill useless ha_event and do some cleanup
  libsas: remove the numbering for each event enum
  libsas: remove unused port_gone_completion and DISCE_PORT_GONE
  libsas: rename notify_port_event() for consistency
  libsas: Use dynamic alloced work to avoid sas event lost
  libsas: shut down the PHY if events reached the threshold
  libsas: make the event threshold configurable
  libsas: Use new workqueue to run sas event and disco event
  libsas: libsas: use flush_workqueue to process disco events
    synchronously
  libsas: direct call probe and destruct

chenxiang (1):
  libsas: add event to defer list tail instead of head when draining

 drivers/scsi/aic94xx/aic94xx_hwi.c    |   3 -
 drivers/scsi/hisi_sas/hisi_sas_main.c |   7 ++-
 drivers/scsi/libsas/sas_ata.c         |   1 -
 drivers/scsi/libsas/sas_discover.c    |  36 +++++++-----
 drivers/scsi/libsas/sas_dump.c        |  10 ----
 drivers/scsi/libsas/sas_dump.h        |   1 -
 drivers/scsi/libsas/sas_event.c       |  97 +++++++++++++++++++-------------
 drivers/scsi/libsas/sas_expander.c    |   2 +-
 drivers/scsi/libsas/sas_init.c        | 101 +++++++++++++++++++++++++++++-----
 drivers/scsi/libsas/sas_internal.h    |   7 +++
 drivers/scsi/libsas/sas_phy.c         |  73 ++++++++++++------------
 drivers/scsi/libsas/sas_port.c        |  25 +++++----
 include/scsi/libsas.h                 |  81 ++++++++++++---------------
 include/scsi/scsi_transport_sas.h     |   1 +
 14 files changed, 270 insertions(+), 175 deletions(-)

-- 
2.5.0

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

* [PATCH v4 01/11] libsas: kill useless ha_event and do some cleanup
  2017-09-06  9:15 [PATCH v4 00/11] Enhance libsas hotplug feature Jason Yan
@ 2017-09-06  9:15 ` Jason Yan
  2017-09-06 12:38   ` Johannes Thumshirn
  2017-09-06 13:20   ` Christoph Hellwig
  2017-09-06  9:15 ` [PATCH v4 02/11] libsas: remove the numbering for each event enum Jason Yan
                   ` (12 subsequent siblings)
  13 siblings, 2 replies; 28+ messages in thread
From: Jason Yan @ 2017-09-06  9:15 UTC (permalink / raw)
  To: martin.petersen, jejb
  Cc: linux-scsi, linux-kernel, john.garry, chenweilong, zhaohongjiang,
	hare, dan.j.williams, jthumshirn, Raj.Dinesh, hch, huangdaode,
	chenxiang66, yanaijie, Ewan Milne, Tomas Henzl

The ha_event now has only one event HAE_RESET, and this
event does nothing. Kill it and do some cleanup.

This is a preparation for enhance libsas hotplug feature in the
next patches.

Signed-off-by: Jason Yan <yanaijie@huawei.com>
Signed-off-by: John Garry <john.garry@huawei.com>
CC: Johannes Thumshirn <jthumshirn@suse.de>
CC: Ewan Milne <emilne@redhat.com>
CC: Christoph Hellwig <hch@lst.de>
CC: Tomas Henzl <thenzl@redhat.com>
CC: Dan Williams <dan.j.williams@intel.com>
---
 drivers/scsi/aic94xx/aic94xx_hwi.c    |  3 ---
 drivers/scsi/hisi_sas/hisi_sas_main.c |  1 -
 drivers/scsi/libsas/sas_dump.c        | 10 ----------
 drivers/scsi/libsas/sas_dump.h        |  1 -
 drivers/scsi/libsas/sas_event.c       | 20 --------------------
 drivers/scsi/libsas/sas_init.c        | 12 ------------
 include/scsi/libsas.h                 | 21 ---------------------
 7 files changed, 68 deletions(-)

diff --git a/drivers/scsi/aic94xx/aic94xx_hwi.c b/drivers/scsi/aic94xx/aic94xx_hwi.c
index f2671a8..ec09438 100644
--- a/drivers/scsi/aic94xx/aic94xx_hwi.c
+++ b/drivers/scsi/aic94xx/aic94xx_hwi.c
@@ -721,11 +721,8 @@ int asd_init_hw(struct asd_ha_struct *asd_ha)
  */
 static void asd_chip_reset(struct asd_ha_struct *asd_ha)
 {
-	struct sas_ha_struct *sas_ha = &asd_ha->sas_ha;
-
 	ASD_DPRINTK("chip reset for %s\n", pci_name(asd_ha->pcidev));
 	asd_chip_hardrst(asd_ha);
-	sas_ha->notify_ha_event(sas_ha, HAE_RESET);
 }
 
 /* ---------- Done List Routines ---------- */
diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c
index 4022c3f..5e47abb 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_main.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
@@ -990,7 +990,6 @@ static int hisi_sas_controller_reset(struct hisi_hba *hisi_hba)
 		hisi_sas_release_tasks(hisi_hba);
 		spin_unlock_irqrestore(&hisi_hba->lock, flags);
 
-		sas_ha->notify_ha_event(sas_ha, HAE_RESET);
 		dev_dbg(dev, "controller reset successful!\n");
 	} else
 		return -1;
diff --git a/drivers/scsi/libsas/sas_dump.c b/drivers/scsi/libsas/sas_dump.c
index cd6f99c..7e5d262 100644
--- a/drivers/scsi/libsas/sas_dump.c
+++ b/drivers/scsi/libsas/sas_dump.c
@@ -24,10 +24,6 @@
 
 #include "sas_dump.h"
 
-static const char *sas_hae_str[] = {
-	[0] = "HAE_RESET",
-};
-
 static const char *sas_porte_str[] = {
 	[0] = "PORTE_BYTES_DMAED",
 	[1] = "PORTE_BROADCAST_RCVD",
@@ -53,12 +49,6 @@ void sas_dprint_phye(int phyid, enum phy_event pe)
 	SAS_DPRINTK("phy%d: phy event: %s\n", phyid, sas_phye_str[pe]);
 }
 
-void sas_dprint_hae(struct sas_ha_struct *sas_ha, enum ha_event he)
-{
-	SAS_DPRINTK("ha %s: %s event\n", dev_name(sas_ha->dev),
-		    sas_hae_str[he]);
-}
-
 void sas_dump_port(struct asd_sas_port *port)
 {
 	SAS_DPRINTK("port%d: class:0x%x\n", port->id, port->class);
diff --git a/drivers/scsi/libsas/sas_dump.h b/drivers/scsi/libsas/sas_dump.h
index 800e4c6..6aaee6b 100644
--- a/drivers/scsi/libsas/sas_dump.h
+++ b/drivers/scsi/libsas/sas_dump.h
@@ -26,5 +26,4 @@
 
 void sas_dprint_porte(int phyid, enum port_event pe);
 void sas_dprint_phye(int phyid, enum phy_event pe);
-void sas_dprint_hae(struct sas_ha_struct *sas_ha, enum ha_event he);
 void sas_dump_port(struct asd_sas_port *port);
diff --git a/drivers/scsi/libsas/sas_event.c b/drivers/scsi/libsas/sas_event.c
index c0d0d97..70c4653 100644
--- a/drivers/scsi/libsas/sas_event.c
+++ b/drivers/scsi/libsas/sas_event.c
@@ -124,14 +124,6 @@ void sas_enable_revalidation(struct sas_ha_struct *ha)
 	mutex_unlock(&ha->disco_mutex);
 }
 
-static int notify_ha_event(struct sas_ha_struct *sas_ha, enum ha_event event)
-{
-	BUG_ON(event >= HA_NUM_EVENTS);
-
-	return sas_queue_event(event, &sas_ha->pending,
-			       &sas_ha->ha_events[event].work, sas_ha);
-}
-
 static int notify_port_event(struct asd_sas_phy *phy, enum port_event event)
 {
 	struct sas_ha_struct *ha = phy->ha;
@@ -154,18 +146,6 @@ int sas_notify_phy_event(struct asd_sas_phy *phy, enum phy_event event)
 
 int sas_init_events(struct sas_ha_struct *sas_ha)
 {
-	static const work_func_t sas_ha_event_fns[HA_NUM_EVENTS] = {
-		[HAE_RESET] = sas_hae_reset,
-	};
-
-	int i;
-
-	for (i = 0; i < HA_NUM_EVENTS; i++) {
-		INIT_SAS_WORK(&sas_ha->ha_events[i].work, sas_ha_event_fns[i]);
-		sas_ha->ha_events[i].ha = sas_ha;
-	}
-
-	sas_ha->notify_ha_event = notify_ha_event;
 	sas_ha->notify_port_event = notify_port_event;
 	sas_ha->notify_phy_event = sas_notify_phy_event;
 
diff --git a/drivers/scsi/libsas/sas_init.c b/drivers/scsi/libsas/sas_init.c
index 64e9cdd..d3f5b57 100644
--- a/drivers/scsi/libsas/sas_init.c
+++ b/drivers/scsi/libsas/sas_init.c
@@ -106,17 +106,6 @@ void sas_hash_addr(u8 *hashed, const u8 *sas_addr)
         hashed[2] = r & 0xFF;
 }
 
-
-/* ---------- HA events ---------- */
-
-void sas_hae_reset(struct work_struct *work)
-{
-	struct sas_ha_event *ev = to_sas_ha_event(work);
-	struct sas_ha_struct *ha = ev->ha;
-
-	clear_bit(HAE_RESET, &ha->pending);
-}
-
 int sas_register_ha(struct sas_ha_struct *sas_ha)
 {
 	int error = 0;
@@ -154,7 +143,6 @@ int sas_register_ha(struct sas_ha_struct *sas_ha)
 	INIT_LIST_HEAD(&sas_ha->eh_ata_q);
 
 	return 0;
-
 Undo_ports:
 	sas_unregister_ports(sas_ha);
 Undo_phys:
diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index cfaeed2..e536597 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -60,11 +60,6 @@ enum sas_phy_type {
  * so when updating/adding events here, please also
  * update the other file too.
  */
-enum ha_event {
-	HAE_RESET             = 0U,
-	HA_NUM_EVENTS         = 1,
-};
-
 enum port_event {
 	PORTE_BYTES_DMAED     = 0U,
 	PORTE_BROADCAST_RCVD  = 1,
@@ -362,18 +357,6 @@ struct scsi_core {
 
 };
 
-struct sas_ha_event {
-	struct sas_work work;
-	struct sas_ha_struct *ha;
-};
-
-static inline struct sas_ha_event *to_sas_ha_event(struct work_struct *work)
-{
-	struct sas_ha_event *ev = container_of(work, typeof(*ev), work.work);
-
-	return ev;
-}
-
 enum sas_ha_state {
 	SAS_HA_REGISTERED,
 	SAS_HA_DRAINING,
@@ -383,9 +366,6 @@ enum sas_ha_state {
 
 struct sas_ha_struct {
 /* private: */
-	struct sas_ha_event ha_events[HA_NUM_EVENTS];
-	unsigned long	 pending;
-
 	struct list_head  defer_q; /* work queued while draining */
 	struct mutex	  drain_mutex;
 	unsigned long	  state;
@@ -415,7 +395,6 @@ struct sas_ha_struct {
 				* their siblings when forming wide ports */
 
 	/* LLDD calls these to notify the class of an event. */
-	int (*notify_ha_event)(struct sas_ha_struct *, enum ha_event);
 	int (*notify_port_event)(struct asd_sas_phy *, enum port_event);
 	int (*notify_phy_event)(struct asd_sas_phy *, enum phy_event);
 
-- 
2.5.0

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

* [PATCH v4 02/11] libsas: remove the numbering for each event enum
  2017-09-06  9:15 [PATCH v4 00/11] Enhance libsas hotplug feature Jason Yan
  2017-09-06  9:15 ` [PATCH v4 01/11] libsas: kill useless ha_event and do some cleanup Jason Yan
@ 2017-09-06  9:15 ` Jason Yan
  2017-09-06 12:41   ` Johannes Thumshirn
  2017-09-06 13:20   ` Christoph Hellwig
  2017-09-06  9:15 ` [PATCH v4 03/11] libsas: remove unused port_gone_completion and DISCE_PORT_GONE Jason Yan
                   ` (11 subsequent siblings)
  13 siblings, 2 replies; 28+ messages in thread
From: Jason Yan @ 2017-09-06  9:15 UTC (permalink / raw)
  To: martin.petersen, jejb
  Cc: linux-scsi, linux-kernel, john.garry, chenweilong, zhaohongjiang,
	hare, dan.j.williams, jthumshirn, Raj.Dinesh, hch, huangdaode,
	chenxiang66, yanaijie, Ewan Milne, Tomas Henzl

Numbering for each event enum makes no sense. Remove the numbering so
that we don't have to calculate the number by hand every time.

Signed-off-by: Jason Yan <yanaijie@huawei.com>
CC: John Garry <john.garry@huawei.com>
CC: Johannes Thumshirn <jthumshirn@suse.de>
CC: Ewan Milne <emilne@redhat.com>
CC: Christoph Hellwig <hch@lst.de>
CC: Tomas Henzl <thenzl@redhat.com>
---
 include/scsi/libsas.h | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index e536597..ccf3b48 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -62,31 +62,31 @@ enum sas_phy_type {
  */
 enum port_event {
 	PORTE_BYTES_DMAED     = 0U,
-	PORTE_BROADCAST_RCVD  = 1,
-	PORTE_LINK_RESET_ERR  = 2,
-	PORTE_TIMER_EVENT     = 3,
-	PORTE_HARD_RESET      = 4,
-	PORT_NUM_EVENTS       = 5,
+	PORTE_BROADCAST_RCVD,
+	PORTE_LINK_RESET_ERR,
+	PORTE_TIMER_EVENT,
+	PORTE_HARD_RESET,
+	PORT_NUM_EVENTS,
 };
 
 enum phy_event {
 	PHYE_LOSS_OF_SIGNAL   = 0U,
-	PHYE_OOB_DONE         = 1,
-	PHYE_OOB_ERROR        = 2,
-	PHYE_SPINUP_HOLD      = 3, /* hot plug SATA, no COMWAKE sent */
-	PHYE_RESUME_TIMEOUT   = 4,
-	PHY_NUM_EVENTS        = 5,
+	PHYE_OOB_DONE,
+	PHYE_OOB_ERROR,
+	PHYE_SPINUP_HOLD,             /* hot plug SATA, no COMWAKE sent */
+	PHYE_RESUME_TIMEOUT,
+	PHY_NUM_EVENTS,
 };
 
 enum discover_event {
 	DISCE_DISCOVER_DOMAIN   = 0U,
-	DISCE_REVALIDATE_DOMAIN = 1,
-	DISCE_PORT_GONE         = 2,
-	DISCE_PROBE		= 3,
-	DISCE_SUSPEND		= 4,
-	DISCE_RESUME		= 5,
-	DISCE_DESTRUCT		= 6,
-	DISC_NUM_EVENTS		= 7,
+	DISCE_REVALIDATE_DOMAIN,
+	DISCE_PORT_GONE,
+	DISCE_PROBE,
+	DISCE_SUSPEND,
+	DISCE_RESUME,
+	DISCE_DESTRUCT,
+	DISC_NUM_EVENTS,
 };
 
 /* ---------- Expander Devices ---------- */
-- 
2.5.0

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

* [PATCH v4 03/11] libsas: remove unused port_gone_completion and DISCE_PORT_GONE
  2017-09-06  9:15 [PATCH v4 00/11] Enhance libsas hotplug feature Jason Yan
  2017-09-06  9:15 ` [PATCH v4 01/11] libsas: kill useless ha_event and do some cleanup Jason Yan
  2017-09-06  9:15 ` [PATCH v4 02/11] libsas: remove the numbering for each event enum Jason Yan
@ 2017-09-06  9:15 ` Jason Yan
  2017-09-06 12:43   ` Johannes Thumshirn
  2017-09-06 13:21   ` Christoph Hellwig
  2017-09-06  9:15 ` [PATCH v4 04/11] libsas: rename notify_port_event() for consistency Jason Yan
                   ` (10 subsequent siblings)
  13 siblings, 2 replies; 28+ messages in thread
From: Jason Yan @ 2017-09-06  9:15 UTC (permalink / raw)
  To: martin.petersen, jejb
  Cc: linux-scsi, linux-kernel, john.garry, chenweilong, zhaohongjiang,
	hare, dan.j.williams, jthumshirn, Raj.Dinesh, hch, huangdaode,
	chenxiang66, yanaijie, Ewan Milne, Tomas Henzl

No one uses the port_gone_completion in struct asd_sas_port and
DISCE_PORT_GONE in enum disover_event, clean them out.

Signed-off-by: Jason Yan <yanaijie@huawei.com>
CC: Johannes Thumshirn <jthumshirn@suse.de>
CC: Ewan Milne <emilne@redhat.com>
CC: Christoph Hellwig <hch@lst.de>
CC: Tomas Henzl <thenzl@redhat.com>
CC: Dan Williams <dan.j.williams@intel.com>
---
 include/scsi/libsas.h | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index ccf3b48..99f32b5 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -81,7 +81,6 @@ enum phy_event {
 enum discover_event {
 	DISCE_DISCOVER_DOMAIN   = 0U,
 	DISCE_REVALIDATE_DOMAIN,
-	DISCE_PORT_GONE,
 	DISCE_PROBE,
 	DISCE_SUSPEND,
 	DISCE_RESUME,
@@ -256,8 +255,6 @@ struct sas_discovery {
 /* The port struct is Class:RW, driver:RO */
 struct asd_sas_port {
 /* private: */
-	struct completion port_gone_completion;
-
 	struct sas_discovery disc;
 	struct domain_device *port_dev;
 	spinlock_t dev_list_lock;
-- 
2.5.0

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

* [PATCH v4 04/11] libsas: rename notify_port_event() for consistency
  2017-09-06  9:15 [PATCH v4 00/11] Enhance libsas hotplug feature Jason Yan
                   ` (2 preceding siblings ...)
  2017-09-06  9:15 ` [PATCH v4 03/11] libsas: remove unused port_gone_completion and DISCE_PORT_GONE Jason Yan
@ 2017-09-06  9:15 ` Jason Yan
  2017-09-06 12:44   ` Johannes Thumshirn
  2017-09-06 13:21   ` Christoph Hellwig
  2017-09-06  9:15 ` [PATCH v4 05/11] libsas: Use dynamic alloced work to avoid sas event lost Jason Yan
                   ` (9 subsequent siblings)
  13 siblings, 2 replies; 28+ messages in thread
From: Jason Yan @ 2017-09-06  9:15 UTC (permalink / raw)
  To: martin.petersen, jejb
  Cc: linux-scsi, linux-kernel, john.garry, chenweilong, zhaohongjiang,
	hare, dan.j.williams, jthumshirn, Raj.Dinesh, hch, huangdaode,
	chenxiang66, yanaijie, Ewan Milne, Tomas Henzl

Rename function notify_port_event() to sas_notify_port_event(), which
will be consistent with sas_notify_phy_event().

Signed-off-by: Jason Yan <yanaijie@huawei.com>
CC: John Garry <john.garry@huawei.com>
CC: Johannes Thumshirn <jthumshirn@suse.de>
CC: Ewan Milne <emilne@redhat.com>
CC: Christoph Hellwig <hch@lst.de>
CC: Tomas Henzl <thenzl@redhat.com>
CC: Dan Williams <dan.j.williams@intel.com>
---
 drivers/scsi/libsas/sas_event.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/libsas/sas_event.c b/drivers/scsi/libsas/sas_event.c
index 70c4653..3e225ef 100644
--- a/drivers/scsi/libsas/sas_event.c
+++ b/drivers/scsi/libsas/sas_event.c
@@ -124,7 +124,7 @@ void sas_enable_revalidation(struct sas_ha_struct *ha)
 	mutex_unlock(&ha->disco_mutex);
 }
 
-static int notify_port_event(struct asd_sas_phy *phy, enum port_event event)
+static int sas_notify_port_event(struct asd_sas_phy *phy, enum port_event event)
 {
 	struct sas_ha_struct *ha = phy->ha;
 
@@ -146,7 +146,7 @@ int sas_notify_phy_event(struct asd_sas_phy *phy, enum phy_event event)
 
 int sas_init_events(struct sas_ha_struct *sas_ha)
 {
-	sas_ha->notify_port_event = notify_port_event;
+	sas_ha->notify_port_event = sas_notify_port_event;
 	sas_ha->notify_phy_event = sas_notify_phy_event;
 
 	return 0;
-- 
2.5.0

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

* [PATCH v4 05/11] libsas: Use dynamic alloced work to avoid sas event lost
  2017-09-06  9:15 [PATCH v4 00/11] Enhance libsas hotplug feature Jason Yan
                   ` (3 preceding siblings ...)
  2017-09-06  9:15 ` [PATCH v4 04/11] libsas: rename notify_port_event() for consistency Jason Yan
@ 2017-09-06  9:15 ` Jason Yan
  2017-09-07  8:22   ` Jack Wang
  2017-09-06  9:15 ` [PATCH v4 06/11] libsas: shut down the PHY if events reached the threshold Jason Yan
                   ` (8 subsequent siblings)
  13 siblings, 1 reply; 28+ messages in thread
From: Jason Yan @ 2017-09-06  9:15 UTC (permalink / raw)
  To: martin.petersen, jejb
  Cc: linux-scsi, linux-kernel, john.garry, chenweilong, zhaohongjiang,
	hare, dan.j.williams, jthumshirn, Raj.Dinesh, hch, huangdaode,
	chenxiang66, yanaijie, Yijing Wang, Ewan Milne, Tomas Henzl

Now libsas hotplug work is static, every sas event type has its own
static work, LLDD driver queues the hotplug work into shost->work_q.
If LLDD driver burst posts lots hotplug events to libsas, the hotplug
events may pending in the workqueue like

shost->work_q
new work[PORTE_BYTES_DMAED] --> |[PHYE_LOSS_OF_SIGNAL][PORTE_BYTES_DMAED] -> processing
                                |<-------wait worker to process-------->|

In this case, a new PORTE_BYTES_DMAED event coming, libsas try to queue
it to shost->work_q, but this work is already pending, so it would be
lost. Finally, libsas delete the related sas port and sas devices, but
LLDD driver expect libsas add the sas port and devices(last sas event).

This patch use dynamic allocated work to avoid this issue.

Signed-off-by: Yijing Wang <wangyijing@huawei.com>
CC: John Garry <john.garry@huawei.com>
CC: Johannes Thumshirn <jthumshirn@suse.de>
CC: Ewan Milne <emilne@redhat.com>
CC: Christoph Hellwig <hch@lst.de>
CC: Tomas Henzl <thenzl@redhat.com>
CC: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Jason Yan <yanaijie@huawei.com>
---
 drivers/scsi/libsas/sas_event.c    | 75 +++++++++++++++++++++++++++++---------
 drivers/scsi/libsas/sas_init.c     | 27 ++++++++++++--
 drivers/scsi/libsas/sas_internal.h |  6 +++
 drivers/scsi/libsas/sas_phy.c      | 44 +++++-----------------
 drivers/scsi/libsas/sas_port.c     | 18 ++++-----
 include/scsi/libsas.h              | 16 +++++---
 6 files changed, 115 insertions(+), 71 deletions(-)

diff --git a/drivers/scsi/libsas/sas_event.c b/drivers/scsi/libsas/sas_event.c
index 3e225ef..35412d9 100644
--- a/drivers/scsi/libsas/sas_event.c
+++ b/drivers/scsi/libsas/sas_event.c
@@ -29,7 +29,8 @@
 
 int sas_queue_work(struct sas_ha_struct *ha, struct sas_work *sw)
 {
-	int rc = 0;
+	/* it's added to the defer_q when draining so return succeed */
+	int rc = 1;
 
 	if (!test_bit(SAS_HA_REGISTERED, &ha->state))
 		return 0;
@@ -44,19 +45,15 @@ int sas_queue_work(struct sas_ha_struct *ha, struct sas_work *sw)
 	return rc;
 }
 
-static int sas_queue_event(int event, unsigned long *pending,
-			    struct sas_work *work,
+static int sas_queue_event(int event, struct sas_work *work,
 			    struct sas_ha_struct *ha)
 {
-	int rc = 0;
+	unsigned long flags;
+	int rc;
 
-	if (!test_and_set_bit(event, pending)) {
-		unsigned long flags;
-
-		spin_lock_irqsave(&ha->lock, flags);
-		rc = sas_queue_work(ha, work);
-		spin_unlock_irqrestore(&ha->lock, flags);
-	}
+	spin_lock_irqsave(&ha->lock, flags);
+	rc = sas_queue_work(ha, work);
+	spin_unlock_irqrestore(&ha->lock, flags);
 
 	return rc;
 }
@@ -66,6 +63,7 @@ void __sas_drain_work(struct sas_ha_struct *ha)
 {
 	struct workqueue_struct *wq = ha->core.shost->work_q;
 	struct sas_work *sw, *_sw;
+	int ret;
 
 	set_bit(SAS_HA_DRAINING, &ha->state);
 	/* flush submitters */
@@ -78,7 +76,11 @@ void __sas_drain_work(struct sas_ha_struct *ha)
 	clear_bit(SAS_HA_DRAINING, &ha->state);
 	list_for_each_entry_safe(sw, _sw, &ha->defer_q, drain_node) {
 		list_del_init(&sw->drain_node);
-		sas_queue_work(ha, sw);
+		ret = sas_queue_work(ha, sw);
+		if (ret != 1) {
+			struct asd_sas_event *ev = to_asd_sas_event(&sw->work);
+			sas_free_event(ev);
+		}
 	}
 	spin_unlock_irq(&ha->lock);
 }
@@ -119,29 +121,68 @@ void sas_enable_revalidation(struct sas_ha_struct *ha)
 		if (!test_and_clear_bit(ev, &d->pending))
 			continue;
 
-		sas_queue_event(ev, &d->pending, &d->disc_work[ev].work, ha);
+		sas_queue_event(ev, &d->disc_work[ev].work, ha);
 	}
 	mutex_unlock(&ha->disco_mutex);
 }
 
+
+static void sas_port_event_worker(struct work_struct *work)
+{
+	struct asd_sas_event *ev = to_asd_sas_event(work);
+
+	sas_port_event_fns[ev->event](work);
+	sas_free_event(ev);
+}
+
+static void sas_phy_event_worker(struct work_struct *work)
+{
+	struct asd_sas_event *ev = to_asd_sas_event(work);
+
+	sas_phy_event_fns[ev->event](work);
+	sas_free_event(ev);
+}
+
 static int sas_notify_port_event(struct asd_sas_phy *phy, enum port_event event)
 {
+	struct asd_sas_event *ev;
 	struct sas_ha_struct *ha = phy->ha;
+	int ret;
 
 	BUG_ON(event >= PORT_NUM_EVENTS);
 
-	return sas_queue_event(event, &phy->port_events_pending,
-			       &phy->port_events[event].work, ha);
+	ev = sas_alloc_event(phy);
+	if (!ev)
+		return -ENOMEM;
+
+	INIT_SAS_EVENT(ev, sas_port_event_worker, phy, event);
+
+	ret = sas_queue_event(event, &ev->work, ha);
+	if (ret != 1)
+		sas_free_event(ev);
+
+	return ret;
 }
 
 int sas_notify_phy_event(struct asd_sas_phy *phy, enum phy_event event)
 {
+	struct asd_sas_event *ev;
 	struct sas_ha_struct *ha = phy->ha;
+	int ret;
 
 	BUG_ON(event >= PHY_NUM_EVENTS);
 
-	return sas_queue_event(event, &phy->phy_events_pending,
-			       &phy->phy_events[event].work, ha);
+	ev = sas_alloc_event(phy);
+	if (!ev)
+		return -ENOMEM;
+
+	INIT_SAS_EVENT(ev, sas_phy_event_worker, phy, event);
+
+	ret = sas_queue_event(event, &ev->work, ha);
+	if (ret != 1)
+		sas_free_event(ev);
+
+	return ret;
 }
 
 int sas_init_events(struct sas_ha_struct *sas_ha)
diff --git a/drivers/scsi/libsas/sas_init.c b/drivers/scsi/libsas/sas_init.c
index d3f5b57..85c278a 100644
--- a/drivers/scsi/libsas/sas_init.c
+++ b/drivers/scsi/libsas/sas_init.c
@@ -39,6 +39,7 @@
 #include "../scsi_sas_internal.h"
 
 static struct kmem_cache *sas_task_cache;
+static struct kmem_cache *sas_event_cache;
 
 struct sas_task *sas_alloc_task(gfp_t flags)
 {
@@ -363,8 +364,6 @@ void sas_prep_resume_ha(struct sas_ha_struct *ha)
 		struct asd_sas_phy *phy = ha->sas_phy[i];
 
 		memset(phy->attached_sas_addr, 0, SAS_ADDR_SIZE);
-		phy->port_events_pending = 0;
-		phy->phy_events_pending = 0;
 		phy->frame_rcvd_size = 0;
 	}
 }
@@ -554,20 +553,42 @@ sas_domain_attach_transport(struct sas_domain_function_template *dft)
 }
 EXPORT_SYMBOL_GPL(sas_domain_attach_transport);
 
+
+struct asd_sas_event *sas_alloc_event(struct asd_sas_phy *phy)
+{
+	gfp_t flags = in_interrupt() ? GFP_ATOMIC : GFP_KERNEL;
+
+	return kmem_cache_zalloc(sas_event_cache, flags);
+}
+
+void sas_free_event(struct asd_sas_event *event)
+{
+	kmem_cache_free(sas_event_cache, event);
+}
+
 /* ---------- SAS Class register/unregister ---------- */
 
 static int __init sas_class_init(void)
 {
 	sas_task_cache = KMEM_CACHE(sas_task, SLAB_HWCACHE_ALIGN);
 	if (!sas_task_cache)
-		return -ENOMEM;
+		goto out;
+
+	sas_event_cache = KMEM_CACHE(asd_sas_event, SLAB_HWCACHE_ALIGN);
+	if (!sas_event_cache)
+		goto free_task_kmem;
 
 	return 0;
+free_task_kmem:
+	kmem_cache_destroy(sas_task_cache);
+out:
+	return -ENOMEM;
 }
 
 static void __exit sas_class_exit(void)
 {
 	kmem_cache_destroy(sas_task_cache);
+	kmem_cache_destroy(sas_event_cache);
 }
 
 MODULE_AUTHOR("Luben Tuikov <luben_tuikov@adaptec.com>");
diff --git a/drivers/scsi/libsas/sas_internal.h b/drivers/scsi/libsas/sas_internal.h
index a216c95..29a7a60 100644
--- a/drivers/scsi/libsas/sas_internal.h
+++ b/drivers/scsi/libsas/sas_internal.h
@@ -61,6 +61,9 @@ int sas_show_oob_mode(enum sas_oob_mode oob_mode, char *buf);
 int  sas_register_phys(struct sas_ha_struct *sas_ha);
 void sas_unregister_phys(struct sas_ha_struct *sas_ha);
 
+struct asd_sas_event *sas_alloc_event(struct asd_sas_phy *phy);
+void sas_free_event(struct asd_sas_event *event);
+
 int  sas_register_ports(struct sas_ha_struct *sas_ha);
 void sas_unregister_ports(struct sas_ha_struct *sas_ha);
 
@@ -97,6 +100,9 @@ void sas_hae_reset(struct work_struct *work);
 
 void sas_free_device(struct kref *kref);
 
+extern const work_func_t sas_phy_event_fns[PHY_NUM_EVENTS];
+extern const work_func_t sas_port_event_fns[PORT_NUM_EVENTS];
+
 #ifdef CONFIG_SCSI_SAS_HOST_SMP
 extern int sas_smp_host_handler(struct Scsi_Host *shost, struct request *req,
 				struct request *rsp);
diff --git a/drivers/scsi/libsas/sas_phy.c b/drivers/scsi/libsas/sas_phy.c
index cdee446c..59f8292 100644
--- a/drivers/scsi/libsas/sas_phy.c
+++ b/drivers/scsi/libsas/sas_phy.c
@@ -35,7 +35,6 @@ static void sas_phye_loss_of_signal(struct work_struct *work)
 	struct asd_sas_event *ev = to_asd_sas_event(work);
 	struct asd_sas_phy *phy = ev->phy;
 
-	clear_bit(PHYE_LOSS_OF_SIGNAL, &phy->phy_events_pending);
 	phy->error = 0;
 	sas_deform_port(phy, 1);
 }
@@ -45,7 +44,6 @@ static void sas_phye_oob_done(struct work_struct *work)
 	struct asd_sas_event *ev = to_asd_sas_event(work);
 	struct asd_sas_phy *phy = ev->phy;
 
-	clear_bit(PHYE_OOB_DONE, &phy->phy_events_pending);
 	phy->error = 0;
 }
 
@@ -58,8 +56,6 @@ static void sas_phye_oob_error(struct work_struct *work)
 	struct sas_internal *i =
 		to_sas_internal(sas_ha->core.shost->transportt);
 
-	clear_bit(PHYE_OOB_ERROR, &phy->phy_events_pending);
-
 	sas_deform_port(phy, 1);
 
 	if (!port && phy->enabled && i->dft->lldd_control_phy) {
@@ -88,8 +84,6 @@ static void sas_phye_spinup_hold(struct work_struct *work)
 	struct sas_internal *i =
 		to_sas_internal(sas_ha->core.shost->transportt);
 
-	clear_bit(PHYE_SPINUP_HOLD, &phy->phy_events_pending);
-
 	phy->error = 0;
 	i->dft->lldd_control_phy(phy, PHY_FUNC_RELEASE_SPINUP_HOLD, NULL);
 }
@@ -99,8 +93,6 @@ static void sas_phye_resume_timeout(struct work_struct *work)
 	struct asd_sas_event *ev = to_asd_sas_event(work);
 	struct asd_sas_phy *phy = ev->phy;
 
-	clear_bit(PHYE_RESUME_TIMEOUT, &phy->phy_events_pending);
-
 	/* phew, lldd got the phy back in the nick of time */
 	if (!phy->suspended) {
 		dev_info(&phy->phy->dev, "resume timeout cancelled\n");
@@ -119,39 +111,12 @@ int sas_register_phys(struct sas_ha_struct *sas_ha)
 {
 	int i;
 
-	static const work_func_t sas_phy_event_fns[PHY_NUM_EVENTS] = {
-		[PHYE_LOSS_OF_SIGNAL] = sas_phye_loss_of_signal,
-		[PHYE_OOB_DONE] = sas_phye_oob_done,
-		[PHYE_OOB_ERROR] = sas_phye_oob_error,
-		[PHYE_SPINUP_HOLD] = sas_phye_spinup_hold,
-		[PHYE_RESUME_TIMEOUT] = sas_phye_resume_timeout,
-
-	};
-
-	static const work_func_t sas_port_event_fns[PORT_NUM_EVENTS] = {
-		[PORTE_BYTES_DMAED] = sas_porte_bytes_dmaed,
-		[PORTE_BROADCAST_RCVD] = sas_porte_broadcast_rcvd,
-		[PORTE_LINK_RESET_ERR] = sas_porte_link_reset_err,
-		[PORTE_TIMER_EVENT] = sas_porte_timer_event,
-		[PORTE_HARD_RESET] = sas_porte_hard_reset,
-	};
-
 	/* Now register the phys. */
 	for (i = 0; i < sas_ha->num_phys; i++) {
-		int k;
 		struct asd_sas_phy *phy = sas_ha->sas_phy[i];
 
 		phy->error = 0;
 		INIT_LIST_HEAD(&phy->port_phy_el);
-		for (k = 0; k < PORT_NUM_EVENTS; k++) {
-			INIT_SAS_WORK(&phy->port_events[k].work, sas_port_event_fns[k]);
-			phy->port_events[k].phy = phy;
-		}
-
-		for (k = 0; k < PHY_NUM_EVENTS; k++) {
-			INIT_SAS_WORK(&phy->phy_events[k].work, sas_phy_event_fns[k]);
-			phy->phy_events[k].phy = phy;
-		}
 
 		phy->port = NULL;
 		phy->ha = sas_ha;
@@ -179,3 +144,12 @@ int sas_register_phys(struct sas_ha_struct *sas_ha)
 
 	return 0;
 }
+
+const work_func_t sas_phy_event_fns[PHY_NUM_EVENTS] = {
+	[PHYE_LOSS_OF_SIGNAL] = sas_phye_loss_of_signal,
+	[PHYE_OOB_DONE] = sas_phye_oob_done,
+	[PHYE_OOB_ERROR] = sas_phye_oob_error,
+	[PHYE_SPINUP_HOLD] = sas_phye_spinup_hold,
+	[PHYE_RESUME_TIMEOUT] = sas_phye_resume_timeout,
+
+};
diff --git a/drivers/scsi/libsas/sas_port.c b/drivers/scsi/libsas/sas_port.c
index d3c5297..9326628 100644
--- a/drivers/scsi/libsas/sas_port.c
+++ b/drivers/scsi/libsas/sas_port.c
@@ -261,8 +261,6 @@ void sas_porte_bytes_dmaed(struct work_struct *work)
 	struct asd_sas_event *ev = to_asd_sas_event(work);
 	struct asd_sas_phy *phy = ev->phy;
 
-	clear_bit(PORTE_BYTES_DMAED, &phy->port_events_pending);
-
 	sas_form_port(phy);
 }
 
@@ -273,8 +271,6 @@ void sas_porte_broadcast_rcvd(struct work_struct *work)
 	unsigned long flags;
 	u32 prim;
 
-	clear_bit(PORTE_BROADCAST_RCVD, &phy->port_events_pending);
-
 	spin_lock_irqsave(&phy->sas_prim_lock, flags);
 	prim = phy->sas_prim;
 	spin_unlock_irqrestore(&phy->sas_prim_lock, flags);
@@ -288,8 +284,6 @@ void sas_porte_link_reset_err(struct work_struct *work)
 	struct asd_sas_event *ev = to_asd_sas_event(work);
 	struct asd_sas_phy *phy = ev->phy;
 
-	clear_bit(PORTE_LINK_RESET_ERR, &phy->port_events_pending);
-
 	sas_deform_port(phy, 1);
 }
 
@@ -298,8 +292,6 @@ void sas_porte_timer_event(struct work_struct *work)
 	struct asd_sas_event *ev = to_asd_sas_event(work);
 	struct asd_sas_phy *phy = ev->phy;
 
-	clear_bit(PORTE_TIMER_EVENT, &phy->port_events_pending);
-
 	sas_deform_port(phy, 1);
 }
 
@@ -308,8 +300,6 @@ void sas_porte_hard_reset(struct work_struct *work)
 	struct asd_sas_event *ev = to_asd_sas_event(work);
 	struct asd_sas_phy *phy = ev->phy;
 
-	clear_bit(PORTE_HARD_RESET, &phy->port_events_pending);
-
 	sas_deform_port(phy, 1);
 }
 
@@ -353,3 +343,11 @@ void sas_unregister_ports(struct sas_ha_struct *sas_ha)
 			sas_deform_port(sas_ha->sas_phy[i], 0);
 
 }
+
+const work_func_t sas_port_event_fns[PORT_NUM_EVENTS] = {
+	[PORTE_BYTES_DMAED] = sas_porte_bytes_dmaed,
+	[PORTE_BROADCAST_RCVD] = sas_porte_broadcast_rcvd,
+	[PORTE_LINK_RESET_ERR] = sas_porte_link_reset_err,
+	[PORTE_TIMER_EVENT] = sas_porte_timer_event,
+	[PORTE_HARD_RESET] = sas_porte_hard_reset,
+};
diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index 99f32b5..c80321b 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -292,6 +292,7 @@ struct asd_sas_port {
 struct asd_sas_event {
 	struct sas_work work;
 	struct asd_sas_phy *phy;
+	int event;
 };
 
 static inline struct asd_sas_event *to_asd_sas_event(struct work_struct *work)
@@ -301,17 +302,20 @@ static inline struct asd_sas_event *to_asd_sas_event(struct work_struct *work)
 	return ev;
 }
 
+static inline void INIT_SAS_EVENT(struct asd_sas_event *ev, void (*fn)(struct work_struct *),
+		struct asd_sas_phy *phy, int event)
+{
+	INIT_SAS_WORK(&ev->work, fn);
+	ev->phy = phy;
+	ev->event = event;
+}
+
+
 /* The phy pretty much is controlled by the LLDD.
  * The class only reads those fields.
  */
 struct asd_sas_phy {
 /* private: */
-	struct asd_sas_event   port_events[PORT_NUM_EVENTS];
-	struct asd_sas_event   phy_events[PHY_NUM_EVENTS];
-
-	unsigned long port_events_pending;
-	unsigned long phy_events_pending;
-
 	int error;
 	int suspended;
 
-- 
2.5.0

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

* [PATCH v4 06/11] libsas: shut down the PHY if events reached the threshold
  2017-09-06  9:15 [PATCH v4 00/11] Enhance libsas hotplug feature Jason Yan
                   ` (4 preceding siblings ...)
  2017-09-06  9:15 ` [PATCH v4 05/11] libsas: Use dynamic alloced work to avoid sas event lost Jason Yan
@ 2017-09-06  9:15 ` Jason Yan
  2017-09-06  9:15 ` [PATCH v4 07/11] libsas: make the event threshold configurable Jason Yan
                   ` (7 subsequent siblings)
  13 siblings, 0 replies; 28+ messages in thread
From: Jason Yan @ 2017-09-06  9:15 UTC (permalink / raw)
  To: martin.petersen, jejb
  Cc: linux-scsi, linux-kernel, john.garry, chenweilong, zhaohongjiang,
	hare, dan.j.williams, jthumshirn, Raj.Dinesh, hch, huangdaode,
	chenxiang66, yanaijie, Ewan Milne, Tomas Henzl

If the PHY burst too many events, we will alloc a lot of events for the
worker. This may leads to memory exhaustion.

Dan Williams suggested to shut down the PHY if the events reached the
threshold, because in this case the PHY may have gone into some
erroneous state. Users can re-enable the PHY by sysfs if they want.

We cannot use the fixed memory pool because if we run out of events, the
shut down event and loss of signal event will lost too. The events still
need to be allocated and processed in this case.

Suggested-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Jason Yan <yanaijie@huawei.com>
CC: John Garry <john.garry@huawei.com>
CC: Johannes Thumshirn <jthumshirn@suse.de>
CC: Ewan Milne <emilne@redhat.com>
CC: Christoph Hellwig <hch@lst.de>
CC: Tomas Henzl <thenzl@redhat.com>
---
 drivers/scsi/libsas/sas_init.c | 21 ++++++++++++++++++++-
 drivers/scsi/libsas/sas_phy.c  | 31 ++++++++++++++++++++++++++++++-
 include/scsi/libsas.h          |  6 ++++++
 3 files changed, 56 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/libsas/sas_init.c b/drivers/scsi/libsas/sas_init.c
index 85c278a..b1e03d5 100644
--- a/drivers/scsi/libsas/sas_init.c
+++ b/drivers/scsi/libsas/sas_init.c
@@ -122,6 +122,8 @@ int sas_register_ha(struct sas_ha_struct *sas_ha)
 	INIT_LIST_HEAD(&sas_ha->defer_q);
 	INIT_LIST_HEAD(&sas_ha->eh_dev_q);
 
+	sas_ha->event_thres = SAS_PHY_SHUTDOWN_THRES;
+
 	error = sas_register_phys(sas_ha);
 	if (error) {
 		printk(KERN_NOTICE "couldn't register sas phys:%d\n", error);
@@ -556,14 +558,31 @@ EXPORT_SYMBOL_GPL(sas_domain_attach_transport);
 
 struct asd_sas_event *sas_alloc_event(struct asd_sas_phy *phy)
 {
+	struct asd_sas_event *event;
 	gfp_t flags = in_interrupt() ? GFP_ATOMIC : GFP_KERNEL;
 
-	return kmem_cache_zalloc(sas_event_cache, flags);
+	event = kmem_cache_zalloc(sas_event_cache, flags);
+	if (!event)
+		return NULL;
+
+	atomic_inc(&phy->event_nr);
+	if (atomic_read(&phy->event_nr) > phy->ha->event_thres &&
+	    !phy->in_shutdown) {
+		phy->in_shutdown = 1;
+		sas_printk("The phy%02d bursting events, shut it down.\n",
+			   phy->id);
+		sas_notify_phy_event(phy, PHYE_SHUTDOWN);
+	}
+
+	return event;
 }
 
 void sas_free_event(struct asd_sas_event *event)
 {
+	struct asd_sas_phy *phy = event->phy;
+
 	kmem_cache_free(sas_event_cache, event);
+	atomic_dec(&phy->event_nr);
 }
 
 /* ---------- SAS Class register/unregister ---------- */
diff --git a/drivers/scsi/libsas/sas_phy.c b/drivers/scsi/libsas/sas_phy.c
index 59f8292..3df1eec 100644
--- a/drivers/scsi/libsas/sas_phy.c
+++ b/drivers/scsi/libsas/sas_phy.c
@@ -35,6 +35,7 @@ static void sas_phye_loss_of_signal(struct work_struct *work)
 	struct asd_sas_event *ev = to_asd_sas_event(work);
 	struct asd_sas_phy *phy = ev->phy;
 
+	phy->in_shutdown = 0;
 	phy->error = 0;
 	sas_deform_port(phy, 1);
 }
@@ -44,6 +45,7 @@ static void sas_phye_oob_done(struct work_struct *work)
 	struct asd_sas_event *ev = to_asd_sas_event(work);
 	struct asd_sas_phy *phy = ev->phy;
 
+	phy->in_shutdown = 0;
 	phy->error = 0;
 }
 
@@ -105,6 +107,32 @@ static void sas_phye_resume_timeout(struct work_struct *work)
 }
 
 
+static void sas_phye_shutdown(struct work_struct *work)
+{
+	struct asd_sas_event *ev = to_asd_sas_event(work);
+	struct asd_sas_phy *phy = ev->phy;
+	struct sas_ha_struct *sas_ha = phy->ha;
+	struct sas_internal *i =
+		to_sas_internal(sas_ha->core.shost->transportt);
+
+	if (phy->enabled && i->dft->lldd_control_phy) {
+		int ret;
+
+		phy->error = 0;
+		phy->enabled = 0;
+		ret = i->dft->lldd_control_phy(phy, PHY_FUNC_DISABLE, NULL);
+		if (ret)
+			sas_printk("lldd disable phy%02d returned %d\n",
+				phy->id, ret);
+
+	} else if (!i->dft->lldd_control_phy)
+		sas_printk("lldd does not support phy%02d control\n", phy->id);
+	else
+		sas_printk("phy%02d is not enabled, cannot shutdown\n",
+			phy->id);
+
+}
+
 /* ---------- Phy class registration ---------- */
 
 int sas_register_phys(struct sas_ha_struct *sas_ha)
@@ -116,6 +144,7 @@ int sas_register_phys(struct sas_ha_struct *sas_ha)
 		struct asd_sas_phy *phy = sas_ha->sas_phy[i];
 
 		phy->error = 0;
+		atomic_set(&phy->event_nr, 0);
 		INIT_LIST_HEAD(&phy->port_phy_el);
 
 		phy->port = NULL;
@@ -151,5 +180,5 @@ const work_func_t sas_phy_event_fns[PHY_NUM_EVENTS] = {
 	[PHYE_OOB_ERROR] = sas_phye_oob_error,
 	[PHYE_SPINUP_HOLD] = sas_phye_spinup_hold,
 	[PHYE_RESUME_TIMEOUT] = sas_phye_resume_timeout,
-
+	[PHYE_SHUTDOWN] = sas_phye_shutdown,
 };
diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index c80321b..2fa0b13 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -75,6 +75,7 @@ enum phy_event {
 	PHYE_OOB_ERROR,
 	PHYE_SPINUP_HOLD,             /* hot plug SATA, no COMWAKE sent */
 	PHYE_RESUME_TIMEOUT,
+	PHYE_SHUTDOWN,
 	PHY_NUM_EVENTS,
 };
 
@@ -310,12 +311,15 @@ static inline void INIT_SAS_EVENT(struct asd_sas_event *ev, void (*fn)(struct wo
 	ev->event = event;
 }
 
+#define SAS_PHY_SHUTDOWN_THRES   1024
 
 /* The phy pretty much is controlled by the LLDD.
  * The class only reads those fields.
  */
 struct asd_sas_phy {
 /* private: */
+	atomic_t event_nr;
+	int in_shutdown;
 	int error;
 	int suspended;
 
@@ -403,6 +407,8 @@ struct sas_ha_struct {
 
 	struct list_head eh_done_q;  /* complete via scsi_eh_flush_done_q */
 	struct list_head eh_ata_q; /* scmds to promote from sas to ata eh */
+
+	int event_thres;
 };
 
 #define SHOST_TO_SAS_HA(_shost) (*(struct sas_ha_struct **)(_shost)->hostdata)
-- 
2.5.0

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

* [PATCH v4 07/11] libsas: make the event threshold configurable
  2017-09-06  9:15 [PATCH v4 00/11] Enhance libsas hotplug feature Jason Yan
                   ` (5 preceding siblings ...)
  2017-09-06  9:15 ` [PATCH v4 06/11] libsas: shut down the PHY if events reached the threshold Jason Yan
@ 2017-09-06  9:15 ` Jason Yan
  2017-09-07  8:15   ` Jack Wang
  2017-09-06  9:15 ` [PATCH v4 08/11] libsas: Use new workqueue to run sas event and disco event Jason Yan
                   ` (6 subsequent siblings)
  13 siblings, 1 reply; 28+ messages in thread
From: Jason Yan @ 2017-09-06  9:15 UTC (permalink / raw)
  To: martin.petersen, jejb
  Cc: linux-scsi, linux-kernel, john.garry, chenweilong, zhaohongjiang,
	hare, dan.j.williams, jthumshirn, Raj.Dinesh, hch, huangdaode,
	chenxiang66, yanaijie, Ewan Milne, Tomas Henzl

Add a sysfs attr that LLDD can configure it for every host. We made
a example in hisi_sas. Other LLDDs using libsas can implement it if
they want.

Suggested-by: Hannes Reinecke <hare@suse.com>
Signed-off-by: Jason Yan <yanaijie@huawei.com>
CC: John Garry <john.garry@huawei.com>
CC: Johannes Thumshirn <jthumshirn@suse.de>
CC: Ewan Milne <emilne@redhat.com>
CC: Christoph Hellwig <hch@lst.de>
CC: Tomas Henzl <thenzl@redhat.com>
CC: Dan Williams <dan.j.williams@intel.com>
---
 drivers/scsi/hisi_sas/hisi_sas_main.c |  6 ++++++
 drivers/scsi/libsas/sas_init.c        | 27 +++++++++++++++++++++++++++
 include/scsi/libsas.h                 |  1 +
 3 files changed, 34 insertions(+)

diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c
index 5e47abb..9eceed1 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_main.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
@@ -1470,6 +1470,11 @@ EXPORT_SYMBOL_GPL(hisi_sas_rescan_topology);
 struct scsi_transport_template *hisi_sas_stt;
 EXPORT_SYMBOL_GPL(hisi_sas_stt);
 
+struct device_attribute *host_attrs[] = {
+    &dev_attr_phy_event_threshold,
+    NULL,
+};
+
 static struct scsi_host_template _hisi_sas_sht = {
 	.module			= THIS_MODULE,
 	.name			= DRV_NAME,
@@ -1489,6 +1494,7 @@ static struct scsi_host_template _hisi_sas_sht = {
 	.eh_bus_reset_handler	= sas_eh_bus_reset_handler,
 	.target_destroy		= sas_target_destroy,
 	.ioctl			= sas_ioctl,
+	.shost_attrs		= host_attrs,
 };
 struct scsi_host_template *hisi_sas_sht = &_hisi_sas_sht;
 EXPORT_SYMBOL_GPL(hisi_sas_sht);
diff --git a/drivers/scsi/libsas/sas_init.c b/drivers/scsi/libsas/sas_init.c
index b1e03d5..e2d98a8 100644
--- a/drivers/scsi/libsas/sas_init.c
+++ b/drivers/scsi/libsas/sas_init.c
@@ -537,6 +537,33 @@ static struct sas_function_template sft = {
 	.smp_handler = sas_smp_handler,
 };
 
+static inline ssize_t phy_event_threshold_show(struct device *dev,
+			struct device_attribute *attr, char *buf)
+{
+	struct Scsi_Host *shost = class_to_shost(dev);
+	struct sas_ha_struct *sha = SHOST_TO_SAS_HA(shost);
+
+	return scnprintf(buf, PAGE_SIZE, "%u\n", sha->event_thres);
+}
+
+static inline ssize_t phy_event_threshold_store(struct device *dev,
+			struct device_attribute *attr,
+			const char *buf, size_t count)
+{
+	struct Scsi_Host *shost = class_to_shost(dev);
+	struct sas_ha_struct *sha = SHOST_TO_SAS_HA(shost);
+
+	sha->event_thres = simple_strtol(buf, NULL, 10);
+
+	return count;
+}
+
+DEVICE_ATTR(phy_event_threshold,
+             S_IRUGO|S_IWUSR,
+             phy_event_threshold_show,
+             phy_event_threshold_store);
+EXPORT_SYMBOL_GPL(dev_attr_phy_event_threshold);
+
 struct scsi_transport_template *
 sas_domain_attach_transport(struct sas_domain_function_template *dft)
 {
diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index 2fa0b13..08c1c32 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -679,6 +679,7 @@ extern int sas_bios_param(struct scsi_device *,
 			  sector_t capacity, int *hsc);
 extern struct scsi_transport_template *
 sas_domain_attach_transport(struct sas_domain_function_template *);
+extern struct device_attribute dev_attr_phy_event_threshold;
 
 int  sas_discover_root_expander(struct domain_device *);
 
-- 
2.5.0

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

* [PATCH v4 08/11] libsas: Use new workqueue to run sas event and disco event
  2017-09-06  9:15 [PATCH v4 00/11] Enhance libsas hotplug feature Jason Yan
                   ` (6 preceding siblings ...)
  2017-09-06  9:15 ` [PATCH v4 07/11] libsas: make the event threshold configurable Jason Yan
@ 2017-09-06  9:15 ` Jason Yan
  2017-09-06  9:15 ` [PATCH v4 09/11] libsas: libsas: use flush_workqueue to process disco events synchronously Jason Yan
                   ` (5 subsequent siblings)
  13 siblings, 0 replies; 28+ messages in thread
From: Jason Yan @ 2017-09-06  9:15 UTC (permalink / raw)
  To: martin.petersen, jejb
  Cc: linux-scsi, linux-kernel, john.garry, chenweilong, zhaohongjiang,
	hare, dan.j.williams, jthumshirn, Raj.Dinesh, hch, huangdaode,
	chenxiang66, yanaijie, Yijing Wang, Ewan Milne, Tomas Henzl

Now all libsas works are queued to scsi host workqueue,
include sas event work post by LLDD and sas discovery
work, and a sas hotplug flow may be divided into several
works, e.g libsas receive a PORTE_BYTES_DMAED event,
currently we process it as following steps:
sas_form_port  --- run in work in shost workq
	sas_discover_domain  --- run in another work in shost workq
		...
		sas_probe_devices  --- run in new work in shost workq
We found during hot-add a device, libsas may need run several
works in same workqueue to add device in system, the process is
not atomic, it may interrupt by other sas event works, like
PHYE_LOSS_OF_SIGNAL.

This patch is preparation of execute libsas sas event in sync. We need
to use different workqueue to run sas event and disco event. Otherwise
the work will be blocked for waiting another chained work in the same
workqueue.

Signed-off-by: Yijing Wang <wangyijing@huawei.com>
CC: John Garry <john.garry@huawei.com>
CC: Johannes Thumshirn <jthumshirn@suse.de>
CC: Ewan Milne <emilne@redhat.com>
CC: Christoph Hellwig <hch@lst.de>
CC: Tomas Henzl <thenzl@redhat.com>
CC: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Jason Yan <yanaijie@huawei.com>
---
 drivers/scsi/libsas/sas_discover.c |  2 +-
 drivers/scsi/libsas/sas_event.c    |  4 ++--
 drivers/scsi/libsas/sas_init.c     | 18 ++++++++++++++++++
 include/scsi/libsas.h              |  3 +++
 4 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/libsas/sas_discover.c b/drivers/scsi/libsas/sas_discover.c
index 60de662..14f714d 100644
--- a/drivers/scsi/libsas/sas_discover.c
+++ b/drivers/scsi/libsas/sas_discover.c
@@ -534,7 +534,7 @@ static void sas_chain_work(struct sas_ha_struct *ha, struct sas_work *sw)
 	 * workqueue, or known to be submitted from a context that is
 	 * not racing against draining
 	 */
-	scsi_queue_work(ha->core.shost, &sw->work);
+	queue_work(ha->disco_q, &sw->work);
 }
 
 static void sas_chain_event(int event, unsigned long *pending,
diff --git a/drivers/scsi/libsas/sas_event.c b/drivers/scsi/libsas/sas_event.c
index 35412d9..c120657 100644
--- a/drivers/scsi/libsas/sas_event.c
+++ b/drivers/scsi/libsas/sas_event.c
@@ -40,7 +40,7 @@ int sas_queue_work(struct sas_ha_struct *ha, struct sas_work *sw)
 		if (list_empty(&sw->drain_node))
 			list_add(&sw->drain_node, &ha->defer_q);
 	} else
-		rc = scsi_queue_work(ha->core.shost, &sw->work);
+		rc = queue_work(ha->event_q, &sw->work);
 
 	return rc;
 }
@@ -61,7 +61,7 @@ static int sas_queue_event(int event, struct sas_work *work,
 
 void __sas_drain_work(struct sas_ha_struct *ha)
 {
-	struct workqueue_struct *wq = ha->core.shost->work_q;
+	struct workqueue_struct *wq = ha->event_q;
 	struct sas_work *sw, *_sw;
 	int ret;
 
diff --git a/drivers/scsi/libsas/sas_init.c b/drivers/scsi/libsas/sas_init.c
index e2d98a8..b49c46f 100644
--- a/drivers/scsi/libsas/sas_init.c
+++ b/drivers/scsi/libsas/sas_init.c
@@ -109,6 +109,7 @@ void sas_hash_addr(u8 *hashed, const u8 *sas_addr)
 
 int sas_register_ha(struct sas_ha_struct *sas_ha)
 {
+	char name[64];
 	int error = 0;
 
 	mutex_init(&sas_ha->disco_mutex);
@@ -142,10 +143,24 @@ int sas_register_ha(struct sas_ha_struct *sas_ha)
 		goto Undo_ports;
 	}
 
+	error = -ENOMEM;
+	snprintf(name, sizeof(name), "%s_event_q", dev_name(sas_ha->dev));
+	sas_ha->event_q = create_singlethread_workqueue(name);
+	if (!sas_ha->event_q)
+		goto Undo_ports;
+
+	snprintf(name, sizeof(name), "%s_disco_q", dev_name(sas_ha->dev));
+	sas_ha->disco_q = create_singlethread_workqueue(name);
+	if (!sas_ha->disco_q)
+		goto Undo_event_q;
+
 	INIT_LIST_HEAD(&sas_ha->eh_done_q);
 	INIT_LIST_HEAD(&sas_ha->eh_ata_q);
 
 	return 0;
+
+Undo_event_q:
+	destroy_workqueue(sas_ha->event_q);
 Undo_ports:
 	sas_unregister_ports(sas_ha);
 Undo_phys:
@@ -176,6 +191,9 @@ int sas_unregister_ha(struct sas_ha_struct *sas_ha)
 	__sas_drain_work(sas_ha);
 	mutex_unlock(&sas_ha->drain_mutex);
 
+	destroy_workqueue(sas_ha->disco_q);
+	destroy_workqueue(sas_ha->event_q);
+
 	return 0;
 }
 
diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index 08c1c32..d1ab157 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -388,6 +388,9 @@ struct sas_ha_struct {
 	struct device *dev;	  /* should be set */
 	struct module *lldd_module; /* should be set */
 
+	struct workqueue_struct *event_q;
+	struct workqueue_struct *disco_q;
+
 	u8 *sas_addr;		  /* must be set */
 	u8 hashed_sas_addr[HASHED_SAS_ADDR_SIZE];
 
-- 
2.5.0

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

* [PATCH v4 09/11] libsas: libsas: use flush_workqueue to process disco events synchronously
  2017-09-06  9:15 [PATCH v4 00/11] Enhance libsas hotplug feature Jason Yan
                   ` (7 preceding siblings ...)
  2017-09-06  9:15 ` [PATCH v4 08/11] libsas: Use new workqueue to run sas event and disco event Jason Yan
@ 2017-09-06  9:15 ` Jason Yan
  2017-09-06  9:15 ` [PATCH v4 10/11] libsas: direct call probe and destruct Jason Yan
                   ` (4 subsequent siblings)
  13 siblings, 0 replies; 28+ messages in thread
From: Jason Yan @ 2017-09-06  9:15 UTC (permalink / raw)
  To: martin.petersen, jejb
  Cc: linux-scsi, linux-kernel, john.garry, chenweilong, zhaohongjiang,
	hare, dan.j.williams, jthumshirn, Raj.Dinesh, hch, huangdaode,
	chenxiang66, yanaijie, Ewan Milne, Tomas Henzl

Use flush_workqueue to insure the disco and revalidate events processed synchronously.

Signed-off-by: Jason Yan <yanaijie@huawei.com>
CC: John Garry <john.garry@huawei.com>
CC: Johannes Thumshirn <jthumshirn@suse.de>
CC: Ewan Milne <emilne@redhat.com>
CC: Christoph Hellwig <hch@lst.de>
CC: Tomas Henzl <thenzl@redhat.com>
CC: Dan Williams <dan.j.williams@intel.com>
---
 drivers/scsi/libsas/sas_port.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/scsi/libsas/sas_port.c b/drivers/scsi/libsas/sas_port.c
index 9326628..64722f4 100644
--- a/drivers/scsi/libsas/sas_port.c
+++ b/drivers/scsi/libsas/sas_port.c
@@ -192,6 +192,7 @@ static void sas_form_port(struct asd_sas_phy *phy)
 		si->dft->lldd_port_formed(phy);
 
 	sas_discover_event(phy->port, DISCE_DISCOVER_DOMAIN);
+	flush_workqueue(sas_ha->disco_q);
 }
 
 /**
@@ -277,6 +278,9 @@ void sas_porte_broadcast_rcvd(struct work_struct *work)
 
 	SAS_DPRINTK("broadcast received: %d\n", prim);
 	sas_discover_event(phy->port, DISCE_REVALIDATE_DOMAIN);
+
+	if (phy->port)
+		flush_workqueue(phy->port->ha->disco_q);
 }
 
 void sas_porte_link_reset_err(struct work_struct *work)
-- 
2.5.0

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

* [PATCH v4 10/11] libsas: direct call probe and destruct
  2017-09-06  9:15 [PATCH v4 00/11] Enhance libsas hotplug feature Jason Yan
                   ` (8 preceding siblings ...)
  2017-09-06  9:15 ` [PATCH v4 09/11] libsas: libsas: use flush_workqueue to process disco events synchronously Jason Yan
@ 2017-09-06  9:15 ` Jason Yan
  2017-09-06  9:15 ` [PATCH v4 11/11] libsas: add event to defer list tail instead of head when draining Jason Yan
                   ` (3 subsequent siblings)
  13 siblings, 0 replies; 28+ messages in thread
From: Jason Yan @ 2017-09-06  9:15 UTC (permalink / raw)
  To: martin.petersen, jejb
  Cc: linux-scsi, linux-kernel, john.garry, chenweilong, zhaohongjiang,
	hare, dan.j.williams, jthumshirn, Raj.Dinesh, hch, huangdaode,
	chenxiang66, yanaijie, Ewan Milne, Tomas Henzl

In commit 87c8331f ([SCSI] libsas: prevent domain rediscovery competing
with ata error handling) introduced disco mutex to prevent rediscovery
competing with ata error handling and put the whole revalidation in the
mutex. But the rphy add/remove needs to wait for the error handling
which also grabs the disco mutex. This may leads to dead lock.So the
probe and destruct event were introduce to do the rphy add/remove
asynchronously and out of the lock.

The asynchronously processed workers makes the whole discovery process
not atomic, the other events may interrupt the process. For example,
if a loss of signal event inserted before the probe event, the
sas_deform_port() is called and the port will be deleted.

And sas_port_delete() may run before the destruct event, but the
port-x:x is the top parent of end device or expander. This leads to
a kernel WARNING such as:

[   82.042979] sysfs group 'power' not found for kobject 'phy-1:0:22'
[   82.042983] ------------[ cut here ]------------
[   82.042986] WARNING: CPU: 54 PID: 1714 at fs/sysfs/group.c:237
sysfs_remove_group+0x94/0xa0
[   82.043059] Call trace:
[   82.043082] [<ffff0000082e7624>] sysfs_remove_group+0x94/0xa0
[   82.043085] [<ffff00000864e320>] dpm_sysfs_remove+0x60/0x70
[   82.043086] [<ffff00000863ee10>] device_del+0x138/0x308
[   82.043089] [<ffff00000869a2d0>] sas_phy_delete+0x38/0x60
[   82.043091] [<ffff00000869a86c>] do_sas_phy_delete+0x6c/0x80
[   82.043093] [<ffff00000863dc20>] device_for_each_child+0x58/0xa0
[   82.043095] [<ffff000008696f80>] sas_remove_children+0x40/0x50
[   82.043100] [<ffff00000869d1bc>] sas_destruct_devices+0x64/0xa0
[   82.043102] [<ffff0000080e93bc>] process_one_work+0x1fc/0x4b0
[   82.043104] [<ffff0000080e96c0>] worker_thread+0x50/0x490
[   82.043105] [<ffff0000080f0364>] kthread+0xfc/0x128
[   82.043107] [<ffff0000080836c0>] ret_from_fork+0x10/0x50

Make probe and destruct a direct call in the disco and revalidate function,
but put them outside the lock. The whole discovery or revalidate won't
be interrupted by other events. And the DISCE_PROBE and DISCE_DESTRUCT
event are deleted as a result of the direct call.

Introduce a new list to destruct the sas_port and put the port delete after
the destruct. This makes sure the right order of destroying the sysfs
kobject and fix the warning above.

Signed-off-by: Jason Yan <yanaijie@huawei.com>
CC: John Garry <john.garry@huawei.com>
CC: Johannes Thumshirn <jthumshirn@suse.de>
CC: Ewan Milne <emilne@redhat.com>
CC: Christoph Hellwig <hch@lst.de>
CC: Tomas Henzl <thenzl@redhat.com>
CC: Dan Williams <dan.j.williams@intel.com>
---
 drivers/scsi/libsas/sas_ata.c      |  1 -
 drivers/scsi/libsas/sas_discover.c | 34 ++++++++++++++++++++--------------
 drivers/scsi/libsas/sas_expander.c |  2 +-
 drivers/scsi/libsas/sas_internal.h |  1 +
 drivers/scsi/libsas/sas_port.c     |  3 +++
 include/scsi/libsas.h              |  3 +--
 include/scsi/scsi_transport_sas.h  |  1 +
 7 files changed, 27 insertions(+), 18 deletions(-)

diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index 87f5e694..dbe8c5e 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -729,7 +729,6 @@ int sas_discover_sata(struct domain_device *dev)
 	if (res)
 		return res;
 
-	sas_discover_event(dev->port, DISCE_PROBE);
 	return 0;
 }
 
diff --git a/drivers/scsi/libsas/sas_discover.c b/drivers/scsi/libsas/sas_discover.c
index 14f714d..d5f5b58 100644
--- a/drivers/scsi/libsas/sas_discover.c
+++ b/drivers/scsi/libsas/sas_discover.c
@@ -212,13 +212,9 @@ void sas_notify_lldd_dev_gone(struct domain_device *dev)
 	}
 }
 
-static void sas_probe_devices(struct work_struct *work)
+static void sas_probe_devices(struct asd_sas_port *port)
 {
 	struct domain_device *dev, *n;
-	struct sas_discovery_event *ev = to_sas_discovery_event(work);
-	struct asd_sas_port *port = ev->port;
-
-	clear_bit(DISCE_PROBE, &port->disc.pending);
 
 	/* devices must be domain members before link recovery and probe */
 	list_for_each_entry(dev, &port->disco_list, disco_list_node) {
@@ -294,7 +290,6 @@ int sas_discover_end_dev(struct domain_device *dev)
 	res = sas_notify_lldd_dev_found(dev);
 	if (res)
 		return res;
-	sas_discover_event(dev->port, DISCE_PROBE);
 
 	return 0;
 }
@@ -353,13 +348,9 @@ static void sas_unregister_common_dev(struct asd_sas_port *port, struct domain_d
 	sas_put_device(dev);
 }
 
-static void sas_destruct_devices(struct work_struct *work)
+void sas_destruct_devices(struct asd_sas_port *port)
 {
 	struct domain_device *dev, *n;
-	struct sas_discovery_event *ev = to_sas_discovery_event(work);
-	struct asd_sas_port *port = ev->port;
-
-	clear_bit(DISCE_DESTRUCT, &port->disc.pending);
 
 	list_for_each_entry_safe(dev, n, &port->destroy_list, disco_list_node) {
 		list_del_init(&dev->disco_list_node);
@@ -370,6 +361,16 @@ static void sas_destruct_devices(struct work_struct *work)
 	}
 }
 
+void sas_destruct_ports(struct asd_sas_port *port)
+{
+	struct sas_port *sas_port, *p;
+
+	list_for_each_entry_safe(sas_port, p, &port->sas_port_del_list, del_list) {
+		list_del_init(&sas_port->del_list);
+		sas_port_delete(sas_port);
+	}
+}
+
 void sas_unregister_dev(struct asd_sas_port *port, struct domain_device *dev)
 {
 	if (!test_bit(SAS_DEV_DESTROY, &dev->state) &&
@@ -384,7 +385,6 @@ void sas_unregister_dev(struct asd_sas_port *port, struct domain_device *dev)
 	if (!test_and_set_bit(SAS_DEV_DESTROY, &dev->state)) {
 		sas_rphy_unlink(dev->rphy);
 		list_move_tail(&dev->disco_list_node, &port->destroy_list);
-		sas_discover_event(dev->port, DISCE_DESTRUCT);
 	}
 }
 
@@ -490,6 +490,8 @@ static void sas_discover_domain(struct work_struct *work)
 		port->port_dev = NULL;
 	}
 
+	sas_probe_devices(port);
+
 	SAS_DPRINTK("DONE DISCOVERY on port %d, pid:%d, result:%d\n", port->id,
 		    task_pid_nr(current), error);
 }
@@ -523,6 +525,12 @@ static void sas_revalidate_domain(struct work_struct *work)
 		    port->id, task_pid_nr(current), res);
  out:
 	mutex_unlock(&ha->disco_mutex);
+
+	sas_destruct_devices(port);
+	sas_destruct_ports(port);
+
+	sas_probe_devices(port);
+
 }
 
 /* ---------- Events ---------- */
@@ -578,10 +586,8 @@ void sas_init_disc(struct sas_discovery *disc, struct asd_sas_port *port)
 	static const work_func_t sas_event_fns[DISC_NUM_EVENTS] = {
 		[DISCE_DISCOVER_DOMAIN] = sas_discover_domain,
 		[DISCE_REVALIDATE_DOMAIN] = sas_revalidate_domain,
-		[DISCE_PROBE] = sas_probe_devices,
 		[DISCE_SUSPEND] = sas_suspend_devices,
 		[DISCE_RESUME] = sas_resume_devices,
-		[DISCE_DESTRUCT] = sas_destruct_devices,
 	};
 
 	disc->pending = 0;
diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index 570b2cb..b107a44 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -1903,7 +1903,7 @@ static void sas_unregister_devs_sas_addr(struct domain_device *parent,
 		sas_port_delete_phy(phy->port, phy->phy);
 		sas_device_set_phy(found, phy->port);
 		if (phy->port->num_phys == 0)
-			sas_port_delete(phy->port);
+			list_add_tail(&phy->port->del_list, &parent->port->sas_port_del_list);
 		phy->port = NULL;
 	}
 }
diff --git a/drivers/scsi/libsas/sas_internal.h b/drivers/scsi/libsas/sas_internal.h
index 29a7a60..dd74ed5 100644
--- a/drivers/scsi/libsas/sas_internal.h
+++ b/drivers/scsi/libsas/sas_internal.h
@@ -99,6 +99,7 @@ int sas_try_ata_reset(struct asd_sas_phy *phy);
 void sas_hae_reset(struct work_struct *work);
 
 void sas_free_device(struct kref *kref);
+void sas_destruct_devices(struct asd_sas_port *port);
 
 extern const work_func_t sas_phy_event_fns[PHY_NUM_EVENTS];
 extern const work_func_t sas_port_event_fns[PORT_NUM_EVENTS];
diff --git a/drivers/scsi/libsas/sas_port.c b/drivers/scsi/libsas/sas_port.c
index 64722f4..f07e55d 100644
--- a/drivers/scsi/libsas/sas_port.c
+++ b/drivers/scsi/libsas/sas_port.c
@@ -66,6 +66,7 @@ static void sas_resume_port(struct asd_sas_phy *phy)
 		rc = sas_notify_lldd_dev_found(dev);
 		if (rc) {
 			sas_unregister_dev(port, dev);
+			sas_destruct_devices(port);
 			continue;
 		}
 
@@ -220,6 +221,7 @@ void sas_deform_port(struct asd_sas_phy *phy, int gone)
 
 	if (port->num_phys == 1) {
 		sas_unregister_domain_devices(port, gone);
+		sas_destruct_devices(port);
 		sas_port_delete(port->port);
 		port->port = NULL;
 	} else {
@@ -317,6 +319,7 @@ static void sas_init_port(struct asd_sas_port *port,
 	INIT_LIST_HEAD(&port->dev_list);
 	INIT_LIST_HEAD(&port->disco_list);
 	INIT_LIST_HEAD(&port->destroy_list);
+	INIT_LIST_HEAD(&port->sas_port_del_list);
 	spin_lock_init(&port->phy_list_lock);
 	INIT_LIST_HEAD(&port->phy_list);
 	port->ha = sas_ha;
diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index d1ab157..cb3b010 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -82,10 +82,8 @@ enum phy_event {
 enum discover_event {
 	DISCE_DISCOVER_DOMAIN   = 0U,
 	DISCE_REVALIDATE_DOMAIN,
-	DISCE_PROBE,
 	DISCE_SUSPEND,
 	DISCE_RESUME,
-	DISCE_DESTRUCT,
 	DISC_NUM_EVENTS,
 };
 
@@ -262,6 +260,7 @@ struct asd_sas_port {
 	struct list_head dev_list;
 	struct list_head disco_list;
 	struct list_head destroy_list;
+	struct list_head sas_port_del_list;
 	enum   sas_linkrate linkrate;
 
 	struct sas_work work;
diff --git a/include/scsi/scsi_transport_sas.h b/include/scsi/scsi_transport_sas.h
index 73d8709..2cf88fa 100644
--- a/include/scsi/scsi_transport_sas.h
+++ b/include/scsi/scsi_transport_sas.h
@@ -154,6 +154,7 @@ struct sas_port {
 
 	struct mutex		phy_list_mutex;
 	struct list_head	phy_list;
+	struct list_head	del_list; /* libsas only */
 };
 
 #define dev_to_sas_port(d) \
-- 
2.5.0

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

* [PATCH v4 11/11] libsas: add event to defer list tail instead of head when draining
  2017-09-06  9:15 [PATCH v4 00/11] Enhance libsas hotplug feature Jason Yan
                   ` (9 preceding siblings ...)
  2017-09-06  9:15 ` [PATCH v4 10/11] libsas: direct call probe and destruct Jason Yan
@ 2017-09-06  9:15 ` Jason Yan
  2017-09-06 13:21   ` Christoph Hellwig
  2017-09-06 13:07 ` [PATCH v4 00/11] Enhance libsas hotplug feature John Garry
                   ` (2 subsequent siblings)
  13 siblings, 1 reply; 28+ messages in thread
From: Jason Yan @ 2017-09-06  9:15 UTC (permalink / raw)
  To: martin.petersen, jejb
  Cc: linux-scsi, linux-kernel, john.garry, chenweilong, zhaohongjiang,
	hare, dan.j.williams, jthumshirn, Raj.Dinesh, hch, huangdaode,
	chenxiang66, yanaijie, Ewan Milne, Tomas Henzl

From: chenxiang <chenxiang66@hisilicon.com>

Events will be added to defer_q list when setting ha->status to
SAS_HA_DRAINING. Events will be called after drain workqueue.

Those events are added to the head of list, but they are scanned one
by one from the head to the tail, which will cause those events be
called in the reverse order of being added. So change list_add to
list_add_tail in function sas_queue_work.

Signed-off-by: chenxiang <chenxiang66@hisilicon.com>
Signed-off-by: Jason Yan <yanaijie@huawei.com>
CC: John Garry <john.garry@huawei.com>
CC: Johannes Thumshirn <jthumshirn@suse.de>
CC: Ewan Milne <emilne@redhat.com>
CC: Christoph Hellwig <hch@lst.de>
CC: Tomas Henzl <thenzl@redhat.com>
CC: Dan Williams <dan.j.williams@intel.com>
---
 drivers/scsi/libsas/sas_event.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/libsas/sas_event.c b/drivers/scsi/libsas/sas_event.c
index c120657..b124198 100644
--- a/drivers/scsi/libsas/sas_event.c
+++ b/drivers/scsi/libsas/sas_event.c
@@ -38,7 +38,7 @@ int sas_queue_work(struct sas_ha_struct *ha, struct sas_work *sw)
 	if (test_bit(SAS_HA_DRAINING, &ha->state)) {
 		/* add it to the defer list, if not already pending */
 		if (list_empty(&sw->drain_node))
-			list_add(&sw->drain_node, &ha->defer_q);
+			list_add_tail(&sw->drain_node, &ha->defer_q);
 	} else
 		rc = queue_work(ha->event_q, &sw->work);
 
-- 
2.5.0

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

* Re: [PATCH v4 01/11] libsas: kill useless ha_event and do some cleanup
  2017-09-06  9:15 ` [PATCH v4 01/11] libsas: kill useless ha_event and do some cleanup Jason Yan
@ 2017-09-06 12:38   ` Johannes Thumshirn
  2017-09-06 13:20   ` Christoph Hellwig
  1 sibling, 0 replies; 28+ messages in thread
From: Johannes Thumshirn @ 2017-09-06 12:38 UTC (permalink / raw)
  To: Jason Yan
  Cc: martin.petersen, jejb, linux-scsi, linux-kernel, john.garry,
	chenweilong, zhaohongjiang, hare, dan.j.williams, Raj.Dinesh,
	hch, huangdaode, chenxiang66, Ewan Milne, Tomas Henzl

Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

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

* Re: [PATCH v4 02/11] libsas: remove the numbering for each event enum
  2017-09-06  9:15 ` [PATCH v4 02/11] libsas: remove the numbering for each event enum Jason Yan
@ 2017-09-06 12:41   ` Johannes Thumshirn
  2017-09-06 13:20   ` Christoph Hellwig
  1 sibling, 0 replies; 28+ messages in thread
From: Johannes Thumshirn @ 2017-09-06 12:41 UTC (permalink / raw)
  To: Jason Yan
  Cc: martin.petersen, jejb, linux-scsi, linux-kernel, john.garry,
	chenweilong, zhaohongjiang, hare, dan.j.williams, Raj.Dinesh,
	hch, huangdaode, chenxiang66, Ewan Milne, Tomas Henzl

I guess it boils down to personal preference, but
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

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

* Re: [PATCH v4 03/11] libsas: remove unused port_gone_completion and DISCE_PORT_GONE
  2017-09-06  9:15 ` [PATCH v4 03/11] libsas: remove unused port_gone_completion and DISCE_PORT_GONE Jason Yan
@ 2017-09-06 12:43   ` Johannes Thumshirn
  2017-09-06 13:21   ` Christoph Hellwig
  1 sibling, 0 replies; 28+ messages in thread
From: Johannes Thumshirn @ 2017-09-06 12:43 UTC (permalink / raw)
  To: Jason Yan
  Cc: martin.petersen, jejb, linux-scsi, linux-kernel, john.garry,
	chenweilong, zhaohongjiang, hare, dan.j.williams, Raj.Dinesh,
	hch, huangdaode, chenxiang66, Ewan Milne, Tomas Henzl


Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

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

* Re: [PATCH v4 04/11] libsas: rename notify_port_event() for consistency
  2017-09-06  9:15 ` [PATCH v4 04/11] libsas: rename notify_port_event() for consistency Jason Yan
@ 2017-09-06 12:44   ` Johannes Thumshirn
  2017-09-06 13:21   ` Christoph Hellwig
  1 sibling, 0 replies; 28+ messages in thread
From: Johannes Thumshirn @ 2017-09-06 12:44 UTC (permalink / raw)
  To: Jason Yan
  Cc: martin.petersen, jejb, linux-scsi, linux-kernel, john.garry,
	chenweilong, zhaohongjiang, hare, dan.j.williams, Raj.Dinesh,
	hch, huangdaode, chenxiang66, Ewan Milne, Tomas Henzl


Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

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

* Re: [PATCH v4 00/11] Enhance libsas hotplug feature
  2017-09-06  9:15 [PATCH v4 00/11] Enhance libsas hotplug feature Jason Yan
                   ` (10 preceding siblings ...)
  2017-09-06  9:15 ` [PATCH v4 11/11] libsas: add event to defer list tail instead of head when draining Jason Yan
@ 2017-09-06 13:07 ` John Garry
  2017-09-06 13:22   ` Christoph Hellwig
  2017-09-16  1:43 ` Martin K. Petersen
  2017-09-18 12:54 ` John Garry
  13 siblings, 1 reply; 28+ messages in thread
From: John Garry @ 2017-09-06 13:07 UTC (permalink / raw)
  To: Jason Yan, martin.petersen, jejb
  Cc: linux-scsi, linux-kernel, chenweilong, zhaohongjiang, hare,
	dan.j.williams, jthumshirn, Raj.Dinesh, hch, huangdaode,
	chenxiang66

On 06/09/2017 10:15, Jason Yan wrote:
> Hello all, Yijing Wang handed over this topic to me. We are working
> on it the last two months. We have tested the patchset for a long
> time. Here is the new version.
>
> Now the libsas hotplug has some issues, Dan Williams report
> a similar bug here before
> https://www.mail-archive.com/linux-scsi@vger.kernel.org/msg39187.html
>
> The issues we have found
> 1. if LLDD burst reports lots of phy-up/phy-down sas events, some events
>    may lost because a same sas events is pending now, finally libsas topo
>    may different the hardware.
> 2. receive a phy down sas event, libsas call sas_deform_port to remove
>    devices, it would first delete the sas port, then put a destruction
>    discovery event in a new work, and queue it at the tail of workqueue,
>    once the sas port be deleted, its children device will be deleted too,
>    when the destruction work start, it will found the target device has
>    been removed, and report a sysfs warnning.
> 3. since a hotplug process will be divided into several works, if a phy up
>    sas event insert into phydown works, like
>    destruction work  ---> PORTE_BYTES_DMAED (sas_form_port) ---->PHYE_LOSS_OF_SIGNAL
>    the hot remove flow would broken by PORTE_BYTES_DMAED event, it's not
>    we expected, and issues would occur.
>
> v3->v4: -get rid of unused ha event and do some cleanup
>         -use dynamic alloced work and support shutting down the phy if active event reached the threshold
> 		-use flush_workqueue instead of wait-completion to process discover events synchronously
> 		-direct call probe and destruct function
> 		-other small code improvements
> v2->v3: some code improvements suggested by Johannes and John,
> 	    split v2 patch 2 into several small patches.
> v1->v2: some code improvements suggested by John Garry
>
> Jason Yan (10):
>   libsas: kill useless ha_event and do some cleanup
>   libsas: remove the numbering for each event enum
>   libsas: remove unused port_gone_completion and DISCE_PORT_GONE
>   libsas: rename notify_port_event() for consistency
>   libsas: Use dynamic alloced work to avoid sas event lost
>   libsas: shut down the PHY if events reached the threshold
>   libsas: make the event threshold configurable
>   libsas: Use new workqueue to run sas event and disco event
>   libsas: libsas: use flush_workqueue to process disco events
>     synchronously
>   libsas: direct call probe and destruct
>
> chenxiang (1):
>   libsas: add event to defer list tail instead of head when draining
>

Regardless of the fate of the rest of the patches in this series, I 
think patches 1,2,3,4,11/11 can be taken in isolation (subject to 
review, of course). It would save maintaining them out-of-tree.

John

>  drivers/scsi/aic94xx/aic94xx_hwi.c    |   3 -
>  drivers/scsi/hisi_sas/hisi_sas_main.c |   7 ++-
>  drivers/scsi/libsas/sas_ata.c         |   1 -
>  drivers/scsi/libsas/sas_discover.c    |  36 +++++++-----
>  drivers/scsi/libsas/sas_dump.c        |  10 ----
>  drivers/scsi/libsas/sas_dump.h        |   1 -
>  drivers/scsi/libsas/sas_event.c       |  97 +++++++++++++++++++-------------
>  drivers/scsi/libsas/sas_expander.c    |   2 +-
>  drivers/scsi/libsas/sas_init.c        | 101 +++++++++++++++++++++++++++++-----
>  drivers/scsi/libsas/sas_internal.h    |   7 +++
>  drivers/scsi/libsas/sas_phy.c         |  73 ++++++++++++------------
>  drivers/scsi/libsas/sas_port.c        |  25 +++++----
>  include/scsi/libsas.h                 |  81 ++++++++++++---------------
>  include/scsi/scsi_transport_sas.h     |   1 +
>  14 files changed, 270 insertions(+), 175 deletions(-)
>

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

* Re: [PATCH v4 01/11] libsas: kill useless ha_event and do some cleanup
  2017-09-06  9:15 ` [PATCH v4 01/11] libsas: kill useless ha_event and do some cleanup Jason Yan
  2017-09-06 12:38   ` Johannes Thumshirn
@ 2017-09-06 13:20   ` Christoph Hellwig
  1 sibling, 0 replies; 28+ messages in thread
From: Christoph Hellwig @ 2017-09-06 13:20 UTC (permalink / raw)
  To: Jason Yan
  Cc: martin.petersen, jejb, linux-scsi, linux-kernel, john.garry,
	chenweilong, zhaohongjiang, hare, dan.j.williams, jthumshirn,
	Raj.Dinesh, hch, huangdaode, chenxiang66, Ewan Milne,
	Tomas Henzl

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

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

* Re: [PATCH v4 02/11] libsas: remove the numbering for each event enum
  2017-09-06  9:15 ` [PATCH v4 02/11] libsas: remove the numbering for each event enum Jason Yan
  2017-09-06 12:41   ` Johannes Thumshirn
@ 2017-09-06 13:20   ` Christoph Hellwig
  1 sibling, 0 replies; 28+ messages in thread
From: Christoph Hellwig @ 2017-09-06 13:20 UTC (permalink / raw)
  To: Jason Yan
  Cc: martin.petersen, jejb, linux-scsi, linux-kernel, john.garry,
	chenweilong, zhaohongjiang, hare, dan.j.williams, jthumshirn,
	Raj.Dinesh, hch, huangdaode, chenxiang66, Ewan Milne,
	Tomas Henzl

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>

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

* Re: [PATCH v4 03/11] libsas: remove unused port_gone_completion and DISCE_PORT_GONE
  2017-09-06  9:15 ` [PATCH v4 03/11] libsas: remove unused port_gone_completion and DISCE_PORT_GONE Jason Yan
  2017-09-06 12:43   ` Johannes Thumshirn
@ 2017-09-06 13:21   ` Christoph Hellwig
  1 sibling, 0 replies; 28+ messages in thread
From: Christoph Hellwig @ 2017-09-06 13:21 UTC (permalink / raw)
  To: Jason Yan
  Cc: martin.petersen, jejb, linux-scsi, linux-kernel, john.garry,
	chenweilong, zhaohongjiang, hare, dan.j.williams, jthumshirn,
	Raj.Dinesh, hch, huangdaode, chenxiang66, Ewan Milne,
	Tomas Henzl

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

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

* Re: [PATCH v4 04/11] libsas: rename notify_port_event() for consistency
  2017-09-06  9:15 ` [PATCH v4 04/11] libsas: rename notify_port_event() for consistency Jason Yan
  2017-09-06 12:44   ` Johannes Thumshirn
@ 2017-09-06 13:21   ` Christoph Hellwig
  1 sibling, 0 replies; 28+ messages in thread
From: Christoph Hellwig @ 2017-09-06 13:21 UTC (permalink / raw)
  To: Jason Yan
  Cc: martin.petersen, jejb, linux-scsi, linux-kernel, john.garry,
	chenweilong, zhaohongjiang, hare, dan.j.williams, jthumshirn,
	Raj.Dinesh, hch, huangdaode, chenxiang66, Ewan Milne,
	Tomas Henzl

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>

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

* Re: [PATCH v4 11/11] libsas: add event to defer list tail instead of head when draining
  2017-09-06  9:15 ` [PATCH v4 11/11] libsas: add event to defer list tail instead of head when draining Jason Yan
@ 2017-09-06 13:21   ` Christoph Hellwig
  0 siblings, 0 replies; 28+ messages in thread
From: Christoph Hellwig @ 2017-09-06 13:21 UTC (permalink / raw)
  To: Jason Yan
  Cc: martin.petersen, jejb, linux-scsi, linux-kernel, john.garry,
	chenweilong, zhaohongjiang, hare, dan.j.williams, jthumshirn,
	Raj.Dinesh, hch, huangdaode, chenxiang66, Ewan Milne,
	Tomas Henzl

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>

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

* Re: [PATCH v4 00/11] Enhance libsas hotplug feature
  2017-09-06 13:07 ` [PATCH v4 00/11] Enhance libsas hotplug feature John Garry
@ 2017-09-06 13:22   ` Christoph Hellwig
  2017-09-07  1:15     ` Jason Yan
  0 siblings, 1 reply; 28+ messages in thread
From: Christoph Hellwig @ 2017-09-06 13:22 UTC (permalink / raw)
  To: John Garry
  Cc: Jason Yan, martin.petersen, jejb, linux-scsi, linux-kernel,
	chenweilong, zhaohongjiang, hare, dan.j.williams, jthumshirn,
	Raj.Dinesh, hch, huangdaode, chenxiang66

On Wed, Sep 06, 2017 at 02:07:57PM +0100, John Garry wrote:
> Regardless of the fate of the rest of the patches in this series, I think 
> patches 1,2,3,4,11/11 can be taken in isolation (subject to review, of 
> course). It would save maintaining them out-of-tree.

I did a quick review of those and they all look fine to me.

I'll try to find some time to review the real changes in the next
days.

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

* Re: [PATCH v4 00/11] Enhance libsas hotplug feature
  2017-09-06 13:22   ` Christoph Hellwig
@ 2017-09-07  1:15     ` Jason Yan
  0 siblings, 0 replies; 28+ messages in thread
From: Jason Yan @ 2017-09-07  1:15 UTC (permalink / raw)
  To: Christoph Hellwig, John Garry
  Cc: martin.petersen, jejb, linux-scsi, linux-kernel, chenweilong,
	zhaohongjiang, hare, dan.j.williams, jthumshirn, Raj.Dinesh,
	huangdaode, chenxiang66



On 2017/9/6 21:22, Christoph Hellwig wrote:
> On Wed, Sep 06, 2017 at 02:07:57PM +0100, John Garry wrote:
>> Regardless of the fate of the rest of the patches in this series, I think
>> patches 1,2,3,4,11/11 can be taken in isolation (subject to review, of
>> course). It would save maintaining them out-of-tree.
>
> I did a quick review of those and they all look fine to me.
>
> I'll try to find some time to review the real changes in the next
> days.
>
> .
>

Thank you very much and I'm looking forward to your suggestions
of the real changes.

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

* Re: [PATCH v4 07/11] libsas: make the event threshold configurable
  2017-09-06  9:15 ` [PATCH v4 07/11] libsas: make the event threshold configurable Jason Yan
@ 2017-09-07  8:15   ` Jack Wang
  0 siblings, 0 replies; 28+ messages in thread
From: Jack Wang @ 2017-09-07  8:15 UTC (permalink / raw)
  To: Jason Yan
  Cc: Martin K. Petersen, jejb, linux-scsi, Linux kernel mailing list,
	john.garry, chenweilong, zhaohongjiang, hare, dan.j.williams,
	Johannes Thumshirn, Raj Dinesh, Christoph Hellwig, huangdaode,
	chenxiang66, Ewan Milne, Tomas Henzl

2017-09-06 11:15 GMT+02:00 Jason Yan <yanaijie@huawei.com>:
> Add a sysfs attr that LLDD can configure it for every host. We made
> a example in hisi_sas. Other LLDDs using libsas can implement it if
> they want.
>
> Suggested-by: Hannes Reinecke <hare@suse.com>
> Signed-off-by: Jason Yan <yanaijie@huawei.com>
> CC: John Garry <john.garry@huawei.com>
> CC: Johannes Thumshirn <jthumshirn@suse.de>
> CC: Ewan Milne <emilne@redhat.com>
> CC: Christoph Hellwig <hch@lst.de>
> CC: Tomas Henzl <thenzl@redhat.com>
> CC: Dan Williams <dan.j.williams@intel.com>
> ---
>  drivers/scsi/hisi_sas/hisi_sas_main.c |  6 ++++++
>  drivers/scsi/libsas/sas_init.c        | 27 +++++++++++++++++++++++++++
>  include/scsi/libsas.h                 |  1 +
>  3 files changed, 34 insertions(+)
>
> diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c
> index 5e47abb..9eceed1 100644
> --- a/drivers/scsi/hisi_sas/hisi_sas_main.c
> +++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
> @@ -1470,6 +1470,11 @@ EXPORT_SYMBOL_GPL(hisi_sas_rescan_topology);
>  struct scsi_transport_template *hisi_sas_stt;
>  EXPORT_SYMBOL_GPL(hisi_sas_stt);
>
> +struct device_attribute *host_attrs[] = {
> +    &dev_attr_phy_event_threshold,
> +    NULL,
> +};
> +
>  static struct scsi_host_template _hisi_sas_sht = {
>         .module                 = THIS_MODULE,
>         .name                   = DRV_NAME,
> @@ -1489,6 +1494,7 @@ static struct scsi_host_template _hisi_sas_sht = {
>         .eh_bus_reset_handler   = sas_eh_bus_reset_handler,
>         .target_destroy         = sas_target_destroy,
>         .ioctl                  = sas_ioctl,
> +       .shost_attrs            = host_attrs,
>  };
>  struct scsi_host_template *hisi_sas_sht = &_hisi_sas_sht;
>  EXPORT_SYMBOL_GPL(hisi_sas_sht);
> diff --git a/drivers/scsi/libsas/sas_init.c b/drivers/scsi/libsas/sas_init.c
> index b1e03d5..e2d98a8 100644
> --- a/drivers/scsi/libsas/sas_init.c
> +++ b/drivers/scsi/libsas/sas_init.c
> @@ -537,6 +537,33 @@ static struct sas_function_template sft = {
>         .smp_handler = sas_smp_handler,
>  };
>
> +static inline ssize_t phy_event_threshold_show(struct device *dev,
> +                       struct device_attribute *attr, char *buf)
> +{
> +       struct Scsi_Host *shost = class_to_shost(dev);
> +       struct sas_ha_struct *sha = SHOST_TO_SAS_HA(shost);
> +
> +       return scnprintf(buf, PAGE_SIZE, "%u\n", sha->event_thres);
> +}
> +
> +static inline ssize_t phy_event_threshold_store(struct device *dev,
> +                       struct device_attribute *attr,
> +                       const char *buf, size_t count)
> +{
> +       struct Scsi_Host *shost = class_to_shost(dev);
> +       struct sas_ha_struct *sha = SHOST_TO_SAS_HA(shost);
> +
> +       sha->event_thres = simple_strtol(buf, NULL, 10);
> +
> +       return count;
> +}
> +
> +DEVICE_ATTR(phy_event_threshold,
> +             S_IRUGO|S_IWUSR,
> +             phy_event_threshold_show,
> +             phy_event_threshold_store);
> +EXPORT_SYMBOL_GPL(dev_attr_phy_event_threshold);
> +
>  struct scsi_transport_template *
>  sas_domain_attach_transport(struct sas_domain_function_template *dft)
>  {
> diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
> index 2fa0b13..08c1c32 100644
> --- a/include/scsi/libsas.h
> +++ b/include/scsi/libsas.h
> @@ -679,6 +679,7 @@ extern int sas_bios_param(struct scsi_device *,
>                           sector_t capacity, int *hsc);
>  extern struct scsi_transport_template *
>  sas_domain_attach_transport(struct sas_domain_function_template *);
> +extern struct device_attribute dev_attr_phy_event_threshold;
>
>  int  sas_discover_root_expander(struct domain_device *);
>
> --
> 2.5.0
>

Looks good, thanks!
Reviewed-by: Jack Wang <jinpu.wang@profitbricks.com>

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

* Re: [PATCH v4 05/11] libsas: Use dynamic alloced work to avoid sas event lost
  2017-09-06  9:15 ` [PATCH v4 05/11] libsas: Use dynamic alloced work to avoid sas event lost Jason Yan
@ 2017-09-07  8:22   ` Jack Wang
  0 siblings, 0 replies; 28+ messages in thread
From: Jack Wang @ 2017-09-07  8:22 UTC (permalink / raw)
  To: Jason Yan
  Cc: Martin K. Petersen, jejb, linux-scsi, Linux kernel mailing list,
	john.garry, chenweilong, zhaohongjiang, hare, dan.j.williams,
	Johannes Thumshirn, Raj Dinesh, Christoph Hellwig, huangdaode,
	chenxiang66, Yijing Wang, Ewan Milne, Tomas Henzl

2017-09-06 11:15 GMT+02:00 Jason Yan <yanaijie@huawei.com>:
> Now libsas hotplug work is static, every sas event type has its own
> static work, LLDD driver queues the hotplug work into shost->work_q.
> If LLDD driver burst posts lots hotplug events to libsas, the hotplug
> events may pending in the workqueue like
>
> shost->work_q
> new work[PORTE_BYTES_DMAED] --> |[PHYE_LOSS_OF_SIGNAL][PORTE_BYTES_DMAED] -> processing
>                                 |<-------wait worker to process-------->|
>
> In this case, a new PORTE_BYTES_DMAED event coming, libsas try to queue
> it to shost->work_q, but this work is already pending, so it would be
> lost. Finally, libsas delete the related sas port and sas devices, but
> LLDD driver expect libsas add the sas port and devices(last sas event).
>
> This patch use dynamic allocated work to avoid this issue.
>
> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
> CC: John Garry <john.garry@huawei.com>
> CC: Johannes Thumshirn <jthumshirn@suse.de>
> CC: Ewan Milne <emilne@redhat.com>
> CC: Christoph Hellwig <hch@lst.de>
> CC: Tomas Henzl <thenzl@redhat.com>
> CC: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Jason Yan <yanaijie@huawei.com>
> ---
>  drivers/scsi/libsas/sas_event.c    | 75 +++++++++++++++++++++++++++++---------
>  drivers/scsi/libsas/sas_init.c     | 27 ++++++++++++--
>  drivers/scsi/libsas/sas_internal.h |  6 +++
>  drivers/scsi/libsas/sas_phy.c      | 44 +++++-----------------
>  drivers/scsi/libsas/sas_port.c     | 18 ++++-----
>  include/scsi/libsas.h              | 16 +++++---
>  6 files changed, 115 insertions(+), 71 deletions(-)
>
> diff --git a/drivers/scsi/libsas/sas_event.c b/drivers/scsi/libsas/sas_event.c
> index 3e225ef..35412d9 100644
> --- a/drivers/scsi/libsas/sas_event.c
> +++ b/drivers/scsi/libsas/sas_event.c
> @@ -29,7 +29,8 @@
>
>  int sas_queue_work(struct sas_ha_struct *ha, struct sas_work *sw)
>  {
> -       int rc = 0;
> +       /* it's added to the defer_q when draining so return succeed */
> +       int rc = 1;
>
>         if (!test_bit(SAS_HA_REGISTERED, &ha->state))
>                 return 0;
> @@ -44,19 +45,15 @@ int sas_queue_work(struct sas_ha_struct *ha, struct sas_work *sw)
>         return rc;
>  }
>
> -static int sas_queue_event(int event, unsigned long *pending,
> -                           struct sas_work *work,
> +static int sas_queue_event(int event, struct sas_work *work,
>                             struct sas_ha_struct *ha)
>  {
> -       int rc = 0;
> +       unsigned long flags;
> +       int rc;
>
> -       if (!test_and_set_bit(event, pending)) {
> -               unsigned long flags;
> -
> -               spin_lock_irqsave(&ha->lock, flags);
> -               rc = sas_queue_work(ha, work);
> -               spin_unlock_irqrestore(&ha->lock, flags);
> -       }
> +       spin_lock_irqsave(&ha->lock, flags);
> +       rc = sas_queue_work(ha, work);
> +       spin_unlock_irqrestore(&ha->lock, flags);
>
>         return rc;
>  }
> @@ -66,6 +63,7 @@ void __sas_drain_work(struct sas_ha_struct *ha)
>  {
>         struct workqueue_struct *wq = ha->core.shost->work_q;
>         struct sas_work *sw, *_sw;
> +       int ret;
>
>         set_bit(SAS_HA_DRAINING, &ha->state);
>         /* flush submitters */
> @@ -78,7 +76,11 @@ void __sas_drain_work(struct sas_ha_struct *ha)
>         clear_bit(SAS_HA_DRAINING, &ha->state);
>         list_for_each_entry_safe(sw, _sw, &ha->defer_q, drain_node) {
>                 list_del_init(&sw->drain_node);
> -               sas_queue_work(ha, sw);
> +               ret = sas_queue_work(ha, sw);
> +               if (ret != 1) {
> +                       struct asd_sas_event *ev = to_asd_sas_event(&sw->work);
> +                       sas_free_event(ev);
> +               }
>         }
>         spin_unlock_irq(&ha->lock);
>  }
> @@ -119,29 +121,68 @@ void sas_enable_revalidation(struct sas_ha_struct *ha)
>                 if (!test_and_clear_bit(ev, &d->pending))
>                         continue;
>
> -               sas_queue_event(ev, &d->pending, &d->disc_work[ev].work, ha);
> +               sas_queue_event(ev, &d->disc_work[ev].work, ha);
>         }
>         mutex_unlock(&ha->disco_mutex);
>  }
>
> +
> +static void sas_port_event_worker(struct work_struct *work)
> +{
> +       struct asd_sas_event *ev = to_asd_sas_event(work);
> +
> +       sas_port_event_fns[ev->event](work);
> +       sas_free_event(ev);
> +}
> +
> +static void sas_phy_event_worker(struct work_struct *work)
> +{
> +       struct asd_sas_event *ev = to_asd_sas_event(work);
> +
> +       sas_phy_event_fns[ev->event](work);
> +       sas_free_event(ev);
> +}
> +
>  static int sas_notify_port_event(struct asd_sas_phy *phy, enum port_event event)
>  {
> +       struct asd_sas_event *ev;
>         struct sas_ha_struct *ha = phy->ha;
> +       int ret;
>
>         BUG_ON(event >= PORT_NUM_EVENTS);
>
> -       return sas_queue_event(event, &phy->port_events_pending,
> -                              &phy->port_events[event].work, ha);
> +       ev = sas_alloc_event(phy);
> +       if (!ev)
> +               return -ENOMEM;
> +
> +       INIT_SAS_EVENT(ev, sas_port_event_worker, phy, event);
> +
> +       ret = sas_queue_event(event, &ev->work, ha);
> +       if (ret != 1)
> +               sas_free_event(ev);
> +
> +       return ret;
>  }
>
>  int sas_notify_phy_event(struct asd_sas_phy *phy, enum phy_event event)
>  {
> +       struct asd_sas_event *ev;
>         struct sas_ha_struct *ha = phy->ha;
> +       int ret;
>
>         BUG_ON(event >= PHY_NUM_EVENTS);
>
> -       return sas_queue_event(event, &phy->phy_events_pending,
> -                              &phy->phy_events[event].work, ha);
> +       ev = sas_alloc_event(phy);
> +       if (!ev)
> +               return -ENOMEM;
> +
> +       INIT_SAS_EVENT(ev, sas_phy_event_worker, phy, event);
> +
> +       ret = sas_queue_event(event, &ev->work, ha);
> +       if (ret != 1)
> +               sas_free_event(ev);
> +
> +       return ret;
>  }
>
>  int sas_init_events(struct sas_ha_struct *sas_ha)
> diff --git a/drivers/scsi/libsas/sas_init.c b/drivers/scsi/libsas/sas_init.c
> index d3f5b57..85c278a 100644
> --- a/drivers/scsi/libsas/sas_init.c
> +++ b/drivers/scsi/libsas/sas_init.c
> @@ -39,6 +39,7 @@
>  #include "../scsi_sas_internal.h"
>
>  static struct kmem_cache *sas_task_cache;
> +static struct kmem_cache *sas_event_cache;
>
>  struct sas_task *sas_alloc_task(gfp_t flags)
>  {
> @@ -363,8 +364,6 @@ void sas_prep_resume_ha(struct sas_ha_struct *ha)
>                 struct asd_sas_phy *phy = ha->sas_phy[i];
>
>                 memset(phy->attached_sas_addr, 0, SAS_ADDR_SIZE);
> -               phy->port_events_pending = 0;
> -               phy->phy_events_pending = 0;
>                 phy->frame_rcvd_size = 0;
>         }
>  }
> @@ -554,20 +553,42 @@ sas_domain_attach_transport(struct sas_domain_function_template *dft)
>  }
>  EXPORT_SYMBOL_GPL(sas_domain_attach_transport);
>
> +
> +struct asd_sas_event *sas_alloc_event(struct asd_sas_phy *phy)
> +{
> +       gfp_t flags = in_interrupt() ? GFP_ATOMIC : GFP_KERNEL;
> +
> +       return kmem_cache_zalloc(sas_event_cache, flags);
> +}
> +
> +void sas_free_event(struct asd_sas_event *event)
> +{
> +       kmem_cache_free(sas_event_cache, event);
> +}
> +
>  /* ---------- SAS Class register/unregister ---------- */
>
>  static int __init sas_class_init(void)
>  {
>         sas_task_cache = KMEM_CACHE(sas_task, SLAB_HWCACHE_ALIGN);
>         if (!sas_task_cache)
> -               return -ENOMEM;
> +               goto out;
> +
> +       sas_event_cache = KMEM_CACHE(asd_sas_event, SLAB_HWCACHE_ALIGN);
> +       if (!sas_event_cache)
> +               goto free_task_kmem;
>
>         return 0;
> +free_task_kmem:
> +       kmem_cache_destroy(sas_task_cache);
> +out:
> +       return -ENOMEM;
>  }
>
>  static void __exit sas_class_exit(void)
>  {
>         kmem_cache_destroy(sas_task_cache);
> +       kmem_cache_destroy(sas_event_cache);
>  }
>
>  MODULE_AUTHOR("Luben Tuikov <luben_tuikov@adaptec.com>");
> diff --git a/drivers/scsi/libsas/sas_internal.h b/drivers/scsi/libsas/sas_internal.h
> index a216c95..29a7a60 100644
> --- a/drivers/scsi/libsas/sas_internal.h
> +++ b/drivers/scsi/libsas/sas_internal.h
> @@ -61,6 +61,9 @@ int sas_show_oob_mode(enum sas_oob_mode oob_mode, char *buf);
>  int  sas_register_phys(struct sas_ha_struct *sas_ha);
>  void sas_unregister_phys(struct sas_ha_struct *sas_ha);
>
> +struct asd_sas_event *sas_alloc_event(struct asd_sas_phy *phy);
> +void sas_free_event(struct asd_sas_event *event);
> +
>  int  sas_register_ports(struct sas_ha_struct *sas_ha);
>  void sas_unregister_ports(struct sas_ha_struct *sas_ha);
>
> @@ -97,6 +100,9 @@ void sas_hae_reset(struct work_struct *work);
>
>  void sas_free_device(struct kref *kref);
>
> +extern const work_func_t sas_phy_event_fns[PHY_NUM_EVENTS];
> +extern const work_func_t sas_port_event_fns[PORT_NUM_EVENTS];
> +
>  #ifdef CONFIG_SCSI_SAS_HOST_SMP
>  extern int sas_smp_host_handler(struct Scsi_Host *shost, struct request *req,
>                                 struct request *rsp);
> diff --git a/drivers/scsi/libsas/sas_phy.c b/drivers/scsi/libsas/sas_phy.c
> index cdee446c..59f8292 100644
> --- a/drivers/scsi/libsas/sas_phy.c
> +++ b/drivers/scsi/libsas/sas_phy.c
> @@ -35,7 +35,6 @@ static void sas_phye_loss_of_signal(struct work_struct *work)
>         struct asd_sas_event *ev = to_asd_sas_event(work);
>         struct asd_sas_phy *phy = ev->phy;
>
> -       clear_bit(PHYE_LOSS_OF_SIGNAL, &phy->phy_events_pending);
>         phy->error = 0;
>         sas_deform_port(phy, 1);
>  }
> @@ -45,7 +44,6 @@ static void sas_phye_oob_done(struct work_struct *work)
>         struct asd_sas_event *ev = to_asd_sas_event(work);
>         struct asd_sas_phy *phy = ev->phy;
>
> -       clear_bit(PHYE_OOB_DONE, &phy->phy_events_pending);
>         phy->error = 0;
>  }
>
> @@ -58,8 +56,6 @@ static void sas_phye_oob_error(struct work_struct *work)
>         struct sas_internal *i =
>                 to_sas_internal(sas_ha->core.shost->transportt);
>
> -       clear_bit(PHYE_OOB_ERROR, &phy->phy_events_pending);
> -
>         sas_deform_port(phy, 1);
>
>         if (!port && phy->enabled && i->dft->lldd_control_phy) {
> @@ -88,8 +84,6 @@ static void sas_phye_spinup_hold(struct work_struct *work)
>         struct sas_internal *i =
>                 to_sas_internal(sas_ha->core.shost->transportt);
>
> -       clear_bit(PHYE_SPINUP_HOLD, &phy->phy_events_pending);
> -
>         phy->error = 0;
>         i->dft->lldd_control_phy(phy, PHY_FUNC_RELEASE_SPINUP_HOLD, NULL);
>  }
> @@ -99,8 +93,6 @@ static void sas_phye_resume_timeout(struct work_struct *work)
>         struct asd_sas_event *ev = to_asd_sas_event(work);
>         struct asd_sas_phy *phy = ev->phy;
>
> -       clear_bit(PHYE_RESUME_TIMEOUT, &phy->phy_events_pending);
> -
>         /* phew, lldd got the phy back in the nick of time */
>         if (!phy->suspended) {
>                 dev_info(&phy->phy->dev, "resume timeout cancelled\n");
> @@ -119,39 +111,12 @@ int sas_register_phys(struct sas_ha_struct *sas_ha)
>  {
>         int i;
>
> -       static const work_func_t sas_phy_event_fns[PHY_NUM_EVENTS] = {
> -               [PHYE_LOSS_OF_SIGNAL] = sas_phye_loss_of_signal,
> -               [PHYE_OOB_DONE] = sas_phye_oob_done,
> -               [PHYE_OOB_ERROR] = sas_phye_oob_error,
> -               [PHYE_SPINUP_HOLD] = sas_phye_spinup_hold,
> -               [PHYE_RESUME_TIMEOUT] = sas_phye_resume_timeout,
> -
> -       };
> -
> -       static const work_func_t sas_port_event_fns[PORT_NUM_EVENTS] = {
> -               [PORTE_BYTES_DMAED] = sas_porte_bytes_dmaed,
> -               [PORTE_BROADCAST_RCVD] = sas_porte_broadcast_rcvd,
> -               [PORTE_LINK_RESET_ERR] = sas_porte_link_reset_err,
> -               [PORTE_TIMER_EVENT] = sas_porte_timer_event,
> -               [PORTE_HARD_RESET] = sas_porte_hard_reset,
> -       };
> -
>         /* Now register the phys. */
>         for (i = 0; i < sas_ha->num_phys; i++) {
> -               int k;
>                 struct asd_sas_phy *phy = sas_ha->sas_phy[i];
>
>                 phy->error = 0;
>                 INIT_LIST_HEAD(&phy->port_phy_el);
> -               for (k = 0; k < PORT_NUM_EVENTS; k++) {
> -                       INIT_SAS_WORK(&phy->port_events[k].work, sas_port_event_fns[k]);
> -                       phy->port_events[k].phy = phy;
> -               }
> -
> -               for (k = 0; k < PHY_NUM_EVENTS; k++) {
> -                       INIT_SAS_WORK(&phy->phy_events[k].work, sas_phy_event_fns[k]);
> -                       phy->phy_events[k].phy = phy;
> -               }
>
>                 phy->port = NULL;
>                 phy->ha = sas_ha;
> @@ -179,3 +144,12 @@ int sas_register_phys(struct sas_ha_struct *sas_ha)
>
>         return 0;
>  }
> +
> +const work_func_t sas_phy_event_fns[PHY_NUM_EVENTS] = {
> +       [PHYE_LOSS_OF_SIGNAL] = sas_phye_loss_of_signal,
> +       [PHYE_OOB_DONE] = sas_phye_oob_done,
> +       [PHYE_OOB_ERROR] = sas_phye_oob_error,
> +       [PHYE_SPINUP_HOLD] = sas_phye_spinup_hold,
> +       [PHYE_RESUME_TIMEOUT] = sas_phye_resume_timeout,
> +
> +};
> diff --git a/drivers/scsi/libsas/sas_port.c b/drivers/scsi/libsas/sas_port.c
> index d3c5297..9326628 100644
> --- a/drivers/scsi/libsas/sas_port.c
> +++ b/drivers/scsi/libsas/sas_port.c
> @@ -261,8 +261,6 @@ void sas_porte_bytes_dmaed(struct work_struct *work)
>         struct asd_sas_event *ev = to_asd_sas_event(work);
>         struct asd_sas_phy *phy = ev->phy;
>
> -       clear_bit(PORTE_BYTES_DMAED, &phy->port_events_pending);
> -
>         sas_form_port(phy);
>  }
>
> @@ -273,8 +271,6 @@ void sas_porte_broadcast_rcvd(struct work_struct *work)
>         unsigned long flags;
>         u32 prim;
>
> -       clear_bit(PORTE_BROADCAST_RCVD, &phy->port_events_pending);
> -
>         spin_lock_irqsave(&phy->sas_prim_lock, flags);
>         prim = phy->sas_prim;
>         spin_unlock_irqrestore(&phy->sas_prim_lock, flags);
> @@ -288,8 +284,6 @@ void sas_porte_link_reset_err(struct work_struct *work)
>         struct asd_sas_event *ev = to_asd_sas_event(work);
>         struct asd_sas_phy *phy = ev->phy;
>
> -       clear_bit(PORTE_LINK_RESET_ERR, &phy->port_events_pending);
> -
>         sas_deform_port(phy, 1);
>  }
>
> @@ -298,8 +292,6 @@ void sas_porte_timer_event(struct work_struct *work)
>         struct asd_sas_event *ev = to_asd_sas_event(work);
>         struct asd_sas_phy *phy = ev->phy;
>
> -       clear_bit(PORTE_TIMER_EVENT, &phy->port_events_pending);
> -
>         sas_deform_port(phy, 1);
>  }
>
> @@ -308,8 +300,6 @@ void sas_porte_hard_reset(struct work_struct *work)
>         struct asd_sas_event *ev = to_asd_sas_event(work);
>         struct asd_sas_phy *phy = ev->phy;
>
> -       clear_bit(PORTE_HARD_RESET, &phy->port_events_pending);
> -
>         sas_deform_port(phy, 1);
>  }
>
> @@ -353,3 +343,11 @@ void sas_unregister_ports(struct sas_ha_struct *sas_ha)
>                         sas_deform_port(sas_ha->sas_phy[i], 0);
>
>  }
> +
> +const work_func_t sas_port_event_fns[PORT_NUM_EVENTS] = {
> +       [PORTE_BYTES_DMAED] = sas_porte_bytes_dmaed,
> +       [PORTE_BROADCAST_RCVD] = sas_porte_broadcast_rcvd,
> +       [PORTE_LINK_RESET_ERR] = sas_porte_link_reset_err,
> +       [PORTE_TIMER_EVENT] = sas_porte_timer_event,
> +       [PORTE_HARD_RESET] = sas_porte_hard_reset,
> +};
> diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
> index 99f32b5..c80321b 100644
> --- a/include/scsi/libsas.h
> +++ b/include/scsi/libsas.h
> @@ -292,6 +292,7 @@ struct asd_sas_port {
>  struct asd_sas_event {
>         struct sas_work work;
>         struct asd_sas_phy *phy;
> +       int event;
>  };
>
>  static inline struct asd_sas_event *to_asd_sas_event(struct work_struct *work)
> @@ -301,17 +302,20 @@ static inline struct asd_sas_event *to_asd_sas_event(struct work_struct *work)
>         return ev;
>  }
>
> +static inline void INIT_SAS_EVENT(struct asd_sas_event *ev, void (*fn)(struct work_struct *),
> +               struct asd_sas_phy *phy, int event)
> +{
> +       INIT_SAS_WORK(&ev->work, fn);
> +       ev->phy = phy;
> +       ev->event = event;
> +}
> +
> +
>  /* The phy pretty much is controlled by the LLDD.
>   * The class only reads those fields.
>   */
>  struct asd_sas_phy {
>  /* private: */
> -       struct asd_sas_event   port_events[PORT_NUM_EVENTS];
> -       struct asd_sas_event   phy_events[PHY_NUM_EVENTS];
> -
> -       unsigned long port_events_pending;
> -       unsigned long phy_events_pending;
> -
>         int error;
>         int suspended;
>
> --
> 2.5.0
>
Looks good, thanks!
Reviewed-by: Jack Wang <jinpu.wang@profitbricks.com>

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

* Re: [PATCH v4 00/11] Enhance libsas hotplug feature
  2017-09-06  9:15 [PATCH v4 00/11] Enhance libsas hotplug feature Jason Yan
                   ` (11 preceding siblings ...)
  2017-09-06 13:07 ` [PATCH v4 00/11] Enhance libsas hotplug feature John Garry
@ 2017-09-16  1:43 ` Martin K. Petersen
  2017-09-18 12:54 ` John Garry
  13 siblings, 0 replies; 28+ messages in thread
From: Martin K. Petersen @ 2017-09-16  1:43 UTC (permalink / raw)
  To: Jason Yan
  Cc: martin.petersen, jejb, linux-scsi, linux-kernel, john.garry,
	chenweilong, zhaohongjiang, hare, dan.j.williams, jthumshirn,
	Raj.Dinesh, hch, huangdaode, chenxiang66


Jason,

> Yijing Wang handed over this topic to me. We are working on it the
> last two months. We have tested the patchset for a long time. Here is
> the new version.

Applied patches 1-4 and 11 to 4.15/scsi-queue. I suggest you resubmit
the rest to get them back on people's radar.

-- 
Martin K. Petersen	Oracle Linux Engineering

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

* Re: [PATCH v4 00/11] Enhance libsas hotplug feature
  2017-09-06  9:15 [PATCH v4 00/11] Enhance libsas hotplug feature Jason Yan
                   ` (12 preceding siblings ...)
  2017-09-16  1:43 ` Martin K. Petersen
@ 2017-09-18 12:54 ` John Garry
  13 siblings, 0 replies; 28+ messages in thread
From: John Garry @ 2017-09-18 12:54 UTC (permalink / raw)
  To: Jason Yan, jack.wang, lindar_liu
  Cc: martin.petersen, jejb, linux-scsi, linux-kernel, chenweilong,
	zhaohongjiang, hare, dan.j.williams, jthumshirn, Raj.Dinesh, hch,
	huangdaode, chenxiang66

On 06/09/2017 10:15, Jason Yan wrote:
> Hello all, Yijing Wang handed over this topic to me. We are working
> on it the last two months. We have tested the patchset for a long
> time. Here is the new version.
>
> Now the libsas hotplug has some issues, Dan Williams report
> a similar bug here before
> https://www.mail-archive.com/linux-scsi@vger.kernel.org/msg39187.html
>
> The issues we have found
> 1. if LLDD burst reports lots of phy-up/phy-down sas events, some events
>    may lost because a same sas events is pending now, finally libsas topo
>    may different the hardware.
> 2. receive a phy down sas event, libsas call sas_deform_port to remove
>    devices, it would first delete the sas port, then put a destruction
>    discovery event in a new work, and queue it at the tail of workqueue,
>    once the sas port be deleted, its children device will be deleted too,
>    when the destruction work start, it will found the target device has
>    been removed, and report a sysfs warnning.
> 3. since a hotplug process will be divided into several works, if a phy up
>    sas event insert into phydown works, like
>    destruction work  ---> PORTE_BYTES_DMAED (sas_form_port) ---->PHYE_LOSS_OF_SIGNAL
>    the hot remove flow would broken by PORTE_BYTES_DMAED event, it's not
>    we expected, and issues would occur.
>

I have tried to verify PM suspend/resume feature with this patchset to 
ensure that it is not broken.

Since our hisi_sas driver does not support PM yet, I have got my hands 
on an adaptec card which uses pm8001 driver (it's vendor 9065, device 
8088) to test.

So in the PM resume, sometimes I find the console locks up with and 
without this patchset. In these cases, I find this log:
[   59.344266] pm80xx pm80xx_chip_init 1098:Firmware is not ready!

Is this a known issue? Should we have a chip reset in all cases in the 
resume code, marked ***:
static int pm8001_pci_resume(struct pci_dev *pdev)
{
...	

	rc = pci_go_44(pdev);
	if (rc)
		goto err_out_disable;

	/* chip soft rst only for spc */
	if (pm8001_ha->chip_id == chip_8001) { **** HERE
		PM8001_CHIP_DISP->chip_soft_rst(pm8001_ha);
		PM8001_INIT_DBG(pm8001_ha,
			pm8001_printk("chip soft reset successful\n"));
	}
	rc = PM8001_CHIP_DISP->chip_init(pm8001_ha);
	if (rc)
		goto err_out_disable;

...
}

John

> v3->v4: -get rid of unused ha event and do some cleanup
>         -use dynamic alloced work and support shutting down the phy if active event reached the threshold
> 		-use flush_workqueue instead of wait-completion to process discover events synchronously
> 		-direct call probe and destruct function
> 		-other small code improvements
> v2->v3: some code improvements suggested by Johannes and John,
> 	    split v2 patch 2 into several small patches.
> v1->v2: some code improvements suggested by John Garry
>
> Jason Yan (10):
>   libsas: kill useless ha_event and do some cleanup
>   libsas: remove the numbering for each event enum
>   libsas: remove unused port_gone_completion and DISCE_PORT_GONE
>   libsas: rename notify_port_event() for consistency
>   libsas: Use dynamic alloced work to avoid sas event lost
>   libsas: shut down the PHY if events reached the threshold
>   libsas: make the event threshold configurable
>   libsas: Use new workqueue to run sas event and disco event
>   libsas: libsas: use flush_workqueue to process disco events
>     synchronously
>   libsas: direct call probe and destruct
>
> chenxiang (1):
>   libsas: add event to defer list tail instead of head when draining
>
>  drivers/scsi/aic94xx/aic94xx_hwi.c    |   3 -
>  drivers/scsi/hisi_sas/hisi_sas_main.c |   7 ++-
>  drivers/scsi/libsas/sas_ata.c         |   1 -
>  drivers/scsi/libsas/sas_discover.c    |  36 +++++++-----
>  drivers/scsi/libsas/sas_dump.c        |  10 ----
>  drivers/scsi/libsas/sas_dump.h        |   1 -
>  drivers/scsi/libsas/sas_event.c       |  97 +++++++++++++++++++-------------
>  drivers/scsi/libsas/sas_expander.c    |   2 +-
>  drivers/scsi/libsas/sas_init.c        | 101 +++++++++++++++++++++++++++++-----
>  drivers/scsi/libsas/sas_internal.h    |   7 +++
>  drivers/scsi/libsas/sas_phy.c         |  73 ++++++++++++------------
>  drivers/scsi/libsas/sas_port.c        |  25 +++++----
>  include/scsi/libsas.h                 |  81 ++++++++++++---------------
>  include/scsi/scsi_transport_sas.h     |   1 +
>  14 files changed, 270 insertions(+), 175 deletions(-)
>

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

end of thread, other threads:[~2017-09-18 12:55 UTC | newest]

Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-06  9:15 [PATCH v4 00/11] Enhance libsas hotplug feature Jason Yan
2017-09-06  9:15 ` [PATCH v4 01/11] libsas: kill useless ha_event and do some cleanup Jason Yan
2017-09-06 12:38   ` Johannes Thumshirn
2017-09-06 13:20   ` Christoph Hellwig
2017-09-06  9:15 ` [PATCH v4 02/11] libsas: remove the numbering for each event enum Jason Yan
2017-09-06 12:41   ` Johannes Thumshirn
2017-09-06 13:20   ` Christoph Hellwig
2017-09-06  9:15 ` [PATCH v4 03/11] libsas: remove unused port_gone_completion and DISCE_PORT_GONE Jason Yan
2017-09-06 12:43   ` Johannes Thumshirn
2017-09-06 13:21   ` Christoph Hellwig
2017-09-06  9:15 ` [PATCH v4 04/11] libsas: rename notify_port_event() for consistency Jason Yan
2017-09-06 12:44   ` Johannes Thumshirn
2017-09-06 13:21   ` Christoph Hellwig
2017-09-06  9:15 ` [PATCH v4 05/11] libsas: Use dynamic alloced work to avoid sas event lost Jason Yan
2017-09-07  8:22   ` Jack Wang
2017-09-06  9:15 ` [PATCH v4 06/11] libsas: shut down the PHY if events reached the threshold Jason Yan
2017-09-06  9:15 ` [PATCH v4 07/11] libsas: make the event threshold configurable Jason Yan
2017-09-07  8:15   ` Jack Wang
2017-09-06  9:15 ` [PATCH v4 08/11] libsas: Use new workqueue to run sas event and disco event Jason Yan
2017-09-06  9:15 ` [PATCH v4 09/11] libsas: libsas: use flush_workqueue to process disco events synchronously Jason Yan
2017-09-06  9:15 ` [PATCH v4 10/11] libsas: direct call probe and destruct Jason Yan
2017-09-06  9:15 ` [PATCH v4 11/11] libsas: add event to defer list tail instead of head when draining Jason Yan
2017-09-06 13:21   ` Christoph Hellwig
2017-09-06 13:07 ` [PATCH v4 00/11] Enhance libsas hotplug feature John Garry
2017-09-06 13:22   ` Christoph Hellwig
2017-09-07  1:15     ` Jason Yan
2017-09-16  1:43 ` Martin K. Petersen
2017-09-18 12:54 ` John Garry

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).