Now the libsas hotplug has some issues, Dan Williams report
a similar bug here before
https://www.mail-archive.com/[email protected]/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 devided 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.
v4->v5: -process only one expander's revalidation in sas_ex_revalidate_domain()
-notify event PORTE_BROADCAST_RCVD in sas_enable_revalidation()
v3->v4: -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
v2->v3: some code improvements suggested by Johannes and John,
split v2 patch 2 into several small pathes.
v1->v2: some code improvements suggested by John Garry
Jason Yan (7):
scsi: libsas: Use dynamic alloced work to avoid sas event lost
scsi: libsas: shut down the PHY if events reached the threshold
scsi: libsas: make the event threshold configurable
scsi: libsas: Use new workqueue to run sas event and disco event
scsi: libsas: use flush_workqueue to process disco events
synchronously
scsi: libsas: direct call probe and destruct
scsi: libsas: notify event PORTE_BROADCAST_RCVD in
sas_enable_revalidation()
drivers/scsi/hisi_sas/hisi_sas_main.c | 6 ++
drivers/scsi/libsas/sas_ata.c | 1 -
drivers/scsi/libsas/sas_discover.c | 34 ++++++-----
drivers/scsi/libsas/sas_event.c | 86 ++++++++++++++++++++-------
drivers/scsi/libsas/sas_expander.c | 8 +--
drivers/scsi/libsas/sas_init.c | 107 +++++++++++++++++++++++++++++++++-
drivers/scsi/libsas/sas_internal.h | 7 +++
drivers/scsi/libsas/sas_phy.c | 69 +++++++++++-----------
drivers/scsi/libsas/sas_port.c | 25 ++++----
include/scsi/libsas.h | 30 +++++++---
include/scsi/scsi_transport_sas.h | 1 +
11 files changed, 277 insertions(+), 97 deletions(-)
--
2.9.5
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 <[email protected]>
CC: John Garry <[email protected]>
CC: Johannes Thumshirn <[email protected]>
CC: Ewan Milne <[email protected]>
CC: Christoph Hellwig <[email protected]>
CC: Tomas Henzl <[email protected]>
CC: Dan Williams <[email protected]>
Signed-off-by: Jason Yan <[email protected]>
---
drivers/scsi/libsas/sas_event.c | 74 +++++++++++++++++++++++++++++---------
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 | 17 +++++----
6 files changed, 115 insertions(+), 71 deletions(-)
diff --git a/drivers/scsi/libsas/sas_event.c b/drivers/scsi/libsas/sas_event.c
index 0bb9eef..5d7254a 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,10 @@ 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)
+ sas_free_event(to_asd_sas_event(&sw->work));
+
}
spin_unlock_irq(&ha->lock);
}
@@ -119,29 +120,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 64fa6f5..e04f6d6 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)
{
@@ -364,8 +365,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;
}
}
@@ -555,20 +554,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 <[email protected]>");
diff --git a/drivers/scsi/libsas/sas_internal.h b/drivers/scsi/libsas/sas_internal.h
index c07e081..d8826a7 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);
@@ -99,6 +102,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 void sas_smp_host_handler(struct bsg_job *job, struct Scsi_Host *shost);
#else
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 6df6fe0..61c84d5 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,21 @@ 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.9.5
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 <[email protected]>
Signed-off-by: Jason Yan <[email protected]>
CC: John Garry <[email protected]>
CC: Johannes Thumshirn <[email protected]>
CC: Ewan Milne <[email protected]>
CC: Christoph Hellwig <[email protected]>
CC: Tomas Henzl <[email protected]>
---
drivers/scsi/libsas/sas_init.c | 33 ++++++++++++++++++++++++++++++++-
drivers/scsi/libsas/sas_phy.c | 27 ++++++++++++++++++++++++++-
include/scsi/libsas.h | 6 ++++++
3 files changed, 64 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/libsas/sas_init.c b/drivers/scsi/libsas/sas_init.c
index e04f6d6..22bfc02 100644
--- a/drivers/scsi/libsas/sas_init.c
+++ b/drivers/scsi/libsas/sas_init.c
@@ -123,6 +123,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);
@@ -557,14 +559,43 @@ 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;
+ struct sas_ha_struct *sas_ha = phy->ha;
+ struct sas_internal *i =
+ to_sas_internal(sas_ha->core.shost->transportt);
+
+ event = kmem_cache_zalloc(sas_event_cache, flags);
+ if (!event)
+ return NULL;
- return kmem_cache_zalloc(sas_event_cache, flags);
+ atomic_inc(&phy->event_nr);
+
+ if (atomic_read(&phy->event_nr) > phy->ha->event_thres) {
+ if (i->dft->lldd_control_phy) {
+ if (cmpxchg(&phy->in_shutdown, 0, 1) == 0) {
+ sas_printk("The phy%02d bursting events, shut it down.\n",
+ phy->id);
+ sas_notify_phy_event(phy, PHYE_SHUTDOWN);
+ }
+ } else {
+ /* Do not support PHY control, stop allocating events */
+ WARN_ONCE(1, "PHY control not supported.\n");
+ kmem_cache_free(sas_event_cache, event);
+ atomic_dec(&phy->event_nr);
+ event = NULL;
+ }
+ }
+
+ 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..bf3e1b9 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,28 @@ 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) {
+ 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
+ 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 +140,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 +176,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 61c84d5..26683e2 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,
};
@@ -311,12 +312,15 @@ static inline void INIT_SAS_EVENT(struct asd_sas_event *ev,
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;
@@ -404,6 +408,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.9.5
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 <[email protected]>
Signed-off-by: Jason Yan <[email protected]>
CC: John Garry <[email protected]>
CC: Johannes Thumshirn <[email protected]>
CC: Ewan Milne <[email protected]>
CC: Christoph Hellwig <[email protected]>
CC: Tomas Henzl <[email protected]>
CC: Dan Williams <[email protected]>
---
drivers/scsi/hisi_sas/hisi_sas_main.c | 6 ++++++
drivers/scsi/libsas/sas_init.c | 31 +++++++++++++++++++++++++++++++
include/scsi/libsas.h | 1 +
3 files changed, 38 insertions(+)
diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c
index 5f503cb..b5ce64a 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_main.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
@@ -1561,6 +1561,11 @@ EXPORT_SYMBOL_GPL(hisi_sas_kill_tasklets);
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,
@@ -1580,6 +1585,7 @@ static struct scsi_host_template _hisi_sas_sht = {
.eh_target_reset_handler = sas_eh_target_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 22bfc02..afd928b 100644
--- a/drivers/scsi/libsas/sas_init.c
+++ b/drivers/scsi/libsas/sas_init.c
@@ -538,6 +538,37 @@ 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);
+
+ /* threshold cannot be set too small */
+ if (sha->event_thres < 32)
+ sha->event_thres = 32;
+
+ 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 26683e2..701c67f 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -681,6 +681,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.9.5
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 <[email protected]>
CC: John Garry <[email protected]>
CC: Johannes Thumshirn <[email protected]>
CC: Ewan Milne <[email protected]>
CC: Christoph Hellwig <[email protected]>
CC: Tomas Henzl <[email protected]>
CC: Dan Williams <[email protected]>
Signed-off-by: Jason Yan <[email protected]>
---
drivers/scsi/libsas/sas_discover.c | 2 +-
drivers/scsi/libsas/sas_event.c | 6 +++---
drivers/scsi/libsas/sas_init.c | 18 ++++++++++++++++++
include/scsi/libsas.h | 3 +++
4 files changed, 25 insertions(+), 4 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 5d7254a..8c82c00 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_tail(&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,6 @@ 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 sas_work *sw, *_sw;
int ret;
@@ -70,7 +69,8 @@ void __sas_drain_work(struct sas_ha_struct *ha)
spin_lock_irq(&ha->lock);
spin_unlock_irq(&ha->lock);
- drain_workqueue(wq);
+ drain_workqueue(ha->event_q);
+ drain_workqueue(ha->disco_q);
spin_lock_irq(&ha->lock);
clear_bit(SAS_HA_DRAINING, &ha->state);
diff --git a/drivers/scsi/libsas/sas_init.c b/drivers/scsi/libsas/sas_init.c
index afd928b..c81a63b 100644
--- a/drivers/scsi/libsas/sas_init.c
+++ b/drivers/scsi/libsas/sas_init.c
@@ -110,6 +110,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);
@@ -143,10 +144,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:
@@ -177,6 +192,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 701c67f..fa93c41 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -389,6 +389,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.9.5
In commit 87c8331fcf72 ("[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.
In sas_ex_revalidate_domain() have a loop to find all broadcasted
device, and sometimes we have a chance to find the same expander twice.
Because the sas_port will be deleted at the end of the whole revalidate
process, sas_port with the same name cannot be added before this.
Otherwise the sysfs will complain of creating duplicate filename. Since
the LLDD will send broadcast for every device change, we can only
process one expander's revalidation.
Signed-off-by: Jason Yan <[email protected]>
CC: John Garry <[email protected]>
CC: Johannes Thumshirn <[email protected]>
CC: Ewan Milne <[email protected]>
CC: Christoph Hellwig <[email protected]>
CC: Tomas Henzl <[email protected]>
CC: Dan Williams <[email protected]>
---
drivers/scsi/libsas/sas_ata.c | 1 -
drivers/scsi/libsas/sas_discover.c | 32 ++++++++++++++++++--------------
drivers/scsi/libsas/sas_expander.c | 8 +++-----
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(+), 22 deletions(-)
diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index 70be442..2b3637b 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -730,7 +730,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..190108e 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,10 @@ 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 +584,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 ca15662..50cb0f3 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -1914,7 +1914,8 @@ 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;
}
}
@@ -2122,7 +2123,7 @@ int sas_ex_revalidate_domain(struct domain_device *port_dev)
struct domain_device *dev = NULL;
res = sas_find_bcast_dev(port_dev, &dev);
- while (res == 0 && dev) {
+ if (res == 0 && dev) {
struct expander_device *ex = &dev->ex_dev;
int i = 0, phy_id;
@@ -2134,9 +2135,6 @@ int sas_ex_revalidate_domain(struct domain_device *port_dev)
res = sas_rediscover(dev, phy_id);
i = phy_id + 1;
} while (i < ex->num_phys);
-
- dev = NULL;
- res = sas_find_bcast_dev(port_dev, &dev);
}
return res;
}
diff --git a/drivers/scsi/libsas/sas_internal.h b/drivers/scsi/libsas/sas_internal.h
index d8826a7..50e12d6 100644
--- a/drivers/scsi/libsas/sas_internal.h
+++ b/drivers/scsi/libsas/sas_internal.h
@@ -101,6 +101,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 fa93c41..225ab77 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 62895b4..05ec927 100644
--- a/include/scsi/scsi_transport_sas.h
+++ b/include/scsi/scsi_transport_sas.h
@@ -156,6 +156,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.9.5
Now we are processing sas event and discover event in different workqueues.
It's safe to wait the discover event done in the sas event work. Use
flush_workqueue() to insure the disco and revalidate events processed
synchronously so that the whole discover and revalidate process will not
be interrupted by other events.
Signed-off-by: Jason Yan <[email protected]>
CC: John Garry <[email protected]>
CC: Johannes Thumshirn <[email protected]>
CC: Ewan Milne <[email protected]>
CC: Christoph Hellwig <[email protected]>
CC: Tomas Henzl <[email protected]>
CC: Dan Williams <[email protected]>
---
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.9.5
There are two places queuing the disco event DISCE_REVALIDATE_DOMAIN.
One is in sas_porte_broadcast_rcvd() and uses sas_chain_event() to queue
the event. The other is in sas_enable_revalidation() and uses
sas_queue_event() to queue the event. We have diffrent work queues for
event and discovery now, so the DISCE_REVALIDATE_DOMAIN event may be
processed in both event queue and discovery queue.
Now since we do synchronous event handling, we cannot do it in discovery
queue, so have to trigger a fake broadcast event to re-trigger the
revalidation from event queue.
Signed-off-by: Jason Yan <[email protected]>
CC: John Garry <[email protected]>
CC: Johannes Thumshirn <[email protected]>
CC: Ewan Milne <[email protected]>
CC: Christoph Hellwig <[email protected]>
CC: Tomas Henzl <[email protected]>
CC: Dan Williams <[email protected]>
---
drivers/scsi/libsas/sas_event.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/scsi/libsas/sas_event.c b/drivers/scsi/libsas/sas_event.c
index 8c82c00..ae923eb 100644
--- a/drivers/scsi/libsas/sas_event.c
+++ b/drivers/scsi/libsas/sas_event.c
@@ -116,11 +116,17 @@ void sas_enable_revalidation(struct sas_ha_struct *ha)
struct asd_sas_port *port = ha->sas_port[i];
const int ev = DISCE_REVALIDATE_DOMAIN;
struct sas_discovery *d = &port->disc;
+ struct asd_sas_phy *sas_phy;
if (!test_and_clear_bit(ev, &d->pending))
continue;
- sas_queue_event(ev, &d->disc_work[ev].work, ha);
+ if (list_empty(&port->phy_list))
+ continue;
+
+ sas_phy = container_of(port->phy_list.next, struct asd_sas_phy,
+ port_phy_el);
+ ha->notify_port_event(sas_phy, PORTE_BROADCAST_RCVD);
}
mutex_unlock(&ha->disco_mutex);
}
--
2.9.5
On 08/12/2017 09:42, Jason Yan wrote:
> 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 <[email protected]>
> Signed-off-by: Jason Yan <[email protected]>
> CC: John Garry <[email protected]>
> CC: Johannes Thumshirn <[email protected]>
> CC: Ewan Milne <[email protected]>
> CC: Christoph Hellwig <[email protected]>
> CC: Tomas Henzl <[email protected]>
> CC: Dan Williams <[email protected]>
Acked-by: John Garry <[email protected]> #for hisi_sas part
> ---
> drivers/scsi/hisi_sas/hisi_sas_main.c | 6 ++++++
On 12/08/2017 10:42 AM, Jason Yan wrote:
> 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 <[email protected]>
> CC: John Garry <[email protected]>
> CC: Johannes Thumshirn <[email protected]>
> CC: Ewan Milne <[email protected]>
> CC: Christoph Hellwig <[email protected]>
> CC: Tomas Henzl <[email protected]>
> CC: Dan Williams <[email protected]>
> Signed-off-by: Jason Yan <[email protected]>
> ---
> drivers/scsi/libsas/sas_event.c | 74 +++++++++++++++++++++++++++++---------
> 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 | 17 +++++----
> 6 files changed, 115 insertions(+), 71 deletions(-)
>
Reviewed-by: Hannes Reinecke <[email protected]>
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
[email protected] +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
On 12/08/2017 10:42 AM, Jason Yan wrote:
> 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 <[email protected]>
> Signed-off-by: Jason Yan <[email protected]>
> CC: John Garry <[email protected]>
> CC: Johannes Thumshirn <[email protected]>
> CC: Ewan Milne <[email protected]>
> CC: Christoph Hellwig <[email protected]>
> CC: Tomas Henzl <[email protected]>
> ---
> drivers/scsi/libsas/sas_init.c | 33 ++++++++++++++++++++++++++++++++-
> drivers/scsi/libsas/sas_phy.c | 27 ++++++++++++++++++++++++++-
> include/scsi/libsas.h | 6 ++++++
> 3 files changed, 64 insertions(+), 2 deletions(-)
>
Well, this still looks a bit error prone; what if the system runs out of
memory before the pool is exhausted?
(Also a threshold of 1024 events is a bit arbitrary; one might want to
adjust that).
Couldn't you allocate two static events always (for shutdown and signal
loss), and then use a fixed pool?
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
[email protected] +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
On 12/08/2017 10:42 AM, Jason Yan wrote:
> 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 <[email protected]>
> Signed-off-by: Jason Yan <[email protected]>
> CC: John Garry <[email protected]>
> CC: Johannes Thumshirn <[email protected]>
> CC: Ewan Milne <[email protected]>
> CC: Christoph Hellwig <[email protected]>
> CC: Tomas Henzl <[email protected]>
> CC: Dan Williams <[email protected]>
> ---
> drivers/scsi/hisi_sas/hisi_sas_main.c | 6 ++++++
> drivers/scsi/libsas/sas_init.c | 31 +++++++++++++++++++++++++++++++
> include/scsi/libsas.h | 1 +
> 3 files changed, 38 insertions(+)
>
Ah, here is it now.
So ignore that part of my previous comment.
Reviewed-by: Hannes Reinecke <[email protected]>
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
[email protected] +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
On 12/08/2017 10:42 AM, Jason Yan wrote:
> 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 <[email protected]>
> CC: John Garry <[email protected]>
> CC: Johannes Thumshirn <[email protected]>
> CC: Ewan Milne <[email protected]>
> CC: Christoph Hellwig <[email protected]>
> CC: Tomas Henzl <[email protected]>
> CC: Dan Williams <[email protected]>
> Signed-off-by: Jason Yan <[email protected]>
> ---
> drivers/scsi/libsas/sas_discover.c | 2 +-
> drivers/scsi/libsas/sas_event.c | 6 +++---
> drivers/scsi/libsas/sas_init.c | 18 ++++++++++++++++++
> include/scsi/libsas.h | 3 +++
> 4 files changed, 25 insertions(+), 4 deletions(-)
>
Reviewed-by: Hannes Reinecke <[email protected]>
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
[email protected] +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
On 12/08/2017 10:42 AM, Jason Yan wrote:
> Now we are processing sas event and discover event in different workqueues.
> It's safe to wait the discover event done in the sas event work. Use
> flush_workqueue() to insure the disco and revalidate events processed
> synchronously so that the whole discover and revalidate process will not
> be interrupted by other events.
>
> Signed-off-by: Jason Yan <[email protected]>
> CC: John Garry <[email protected]>
> CC: Johannes Thumshirn <[email protected]>
> CC: Ewan Milne <[email protected]>
> CC: Christoph Hellwig <[email protected]>
> CC: Tomas Henzl <[email protected]>
> CC: Dan Williams <[email protected]>
> ---
> 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)
>
Reviewed-by: Hannes Reinecke <[email protected]>
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
[email protected] +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
On 12/08/2017 10:42 AM, Jason Yan wrote:
> In commit 87c8331fcf72 ("[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.
>
> In sas_ex_revalidate_domain() have a loop to find all broadcasted
> device, and sometimes we have a chance to find the same expander twice.
> Because the sas_port will be deleted at the end of the whole revalidate
> process, sas_port with the same name cannot be added before this.
> Otherwise the sysfs will complain of creating duplicate filename. Since
> the LLDD will send broadcast for every device change, we can only
> process one expander's revalidation.
>
> Signed-off-by: Jason Yan <[email protected]>
> CC: John Garry <[email protected]>
> CC: Johannes Thumshirn <[email protected]>
> CC: Ewan Milne <[email protected]>
> CC: Christoph Hellwig <[email protected]>
> CC: Tomas Henzl <[email protected]>
> CC: Dan Williams <[email protected]>
> ---
> drivers/scsi/libsas/sas_ata.c | 1 -
> drivers/scsi/libsas/sas_discover.c | 32 ++++++++++++++++++--------------
> drivers/scsi/libsas/sas_expander.c | 8 +++-----
> 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(+), 22 deletions(-)
>
Signed-off-by: Hannes Reinecke <[email protected]>
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
[email protected] +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
On 12/08/2017 10:42 AM, Jason Yan wrote:
> There are two places queuing the disco event DISCE_REVALIDATE_DOMAIN.
> One is in sas_porte_broadcast_rcvd() and uses sas_chain_event() to queue
> the event. The other is in sas_enable_revalidation() and uses
> sas_queue_event() to queue the event. We have diffrent work queues for
> event and discovery now, so the DISCE_REVALIDATE_DOMAIN event may be
> processed in both event queue and discovery queue.
>
> Now since we do synchronous event handling, we cannot do it in discovery
> queue, so have to trigger a fake broadcast event to re-trigger the
> revalidation from event queue.
>
> Signed-off-by: Jason Yan <[email protected]>
> CC: John Garry <[email protected]>
> CC: Johannes Thumshirn <[email protected]>
> CC: Ewan Milne <[email protected]>
> CC: Christoph Hellwig <[email protected]>
> CC: Tomas Henzl <[email protected]>
> CC: Dan Williams <[email protected]>
> ---
> drivers/scsi/libsas/sas_event.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/libsas/sas_event.c b/drivers/scsi/libsas/sas_event.c
> index 8c82c00..ae923eb 100644
> --- a/drivers/scsi/libsas/sas_event.c
> +++ b/drivers/scsi/libsas/sas_event.c
> @@ -116,11 +116,17 @@ void sas_enable_revalidation(struct sas_ha_struct *ha)
> struct asd_sas_port *port = ha->sas_port[i];
> const int ev = DISCE_REVALIDATE_DOMAIN;
> struct sas_discovery *d = &port->disc;
> + struct asd_sas_phy *sas_phy;
>
> if (!test_and_clear_bit(ev, &d->pending))
> continue;
>
> - sas_queue_event(ev, &d->disc_work[ev].work, ha);
> + if (list_empty(&port->phy_list))
> + continue;
> +
> + sas_phy = container_of(port->phy_list.next, struct asd_sas_phy,
> + port_phy_el);
> + ha->notify_port_event(sas_phy, PORTE_BROADCAST_RCVD);
> }
> mutex_unlock(&ha->disco_mutex);
> }
>
Reviewed-by: Hannes Reinecke <[email protected]>
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
[email protected] +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
On 08/12/2017 09:42, Jason Yan wrote:
> Now the libsas hotplug has some issues, Dan Williams report
> a similar bug here before
> https://www.mail-archive.com/[email protected]/msg39187.html
>
Hi Martin, James,
At this point we feel that we have a decent solution to the
long-standing libsas hotplug issues.
Hannes has kindly reviewed the series.
Can you let us know what else you require for acceptance? More
independent review or testing?
Thanks,
John
> 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 devided 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.
>
> v4->v5: -process only one expander's revalidation in sas_ex_revalidate_domain()
> -notify event PORTE_BROADCAST_RCVD in sas_enable_revalidation()
> v3->v4: -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
> v2->v3: some code improvements suggested by Johannes and John,
> split v2 patch 2 into several small pathes.
> v1->v2: some code improvements suggested by John Garry
>
> Jason Yan (7):
> scsi: libsas: Use dynamic alloced work to avoid sas event lost
> scsi: libsas: shut down the PHY if events reached the threshold
> scsi: libsas: make the event threshold configurable
> scsi: libsas: Use new workqueue to run sas event and disco event
> scsi: libsas: use flush_workqueue to process disco events
> synchronously
> scsi: libsas: direct call probe and destruct
> scsi: libsas: notify event PORTE_BROADCAST_RCVD in
> sas_enable_revalidation()
>
> drivers/scsi/hisi_sas/hisi_sas_main.c | 6 ++
> drivers/scsi/libsas/sas_ata.c | 1 -
> drivers/scsi/libsas/sas_discover.c | 34 ++++++-----
> drivers/scsi/libsas/sas_event.c | 86 ++++++++++++++++++++-------
> drivers/scsi/libsas/sas_expander.c | 8 +--
> drivers/scsi/libsas/sas_init.c | 107 +++++++++++++++++++++++++++++++++-
> drivers/scsi/libsas/sas_internal.h | 7 +++
> drivers/scsi/libsas/sas_phy.c | 69 +++++++++++-----------
> drivers/scsi/libsas/sas_port.c | 25 ++++----
> include/scsi/libsas.h | 30 +++++++---
> include/scsi/scsi_transport_sas.h | 1 +
> 11 files changed, 277 insertions(+), 97 deletions(-)
>
John,
> At this point we feel that we have a decent solution to the
> long-standing libsas hotplug issues.
>
> Hannes has kindly reviewed the series.
>
> Can you let us know what else you require for acceptance? More
> independent review or testing?
According to my notes, Hannes had some concerns wrt. one of the
patches. That's why I didn't merge the series.
Please address the comments and we'll take it from there.
--
Martin K. Petersen Oracle Linux Engineering
On 15/12/2017 12:19, Hannes Reinecke wrote:
> On 12/08/2017 10:42 AM, Jason Yan wrote:
>> 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 <[email protected]>
>> Signed-off-by: Jason Yan <[email protected]>
>> CC: John Garry <[email protected]>
>> CC: Johannes Thumshirn <[email protected]>
>> CC: Ewan Milne <[email protected]>
>> CC: Christoph Hellwig <[email protected]>
>> CC: Tomas Henzl <[email protected]>
>> CC: Dan Williams <[email protected]>
>> ---
>> drivers/scsi/hisi_sas/hisi_sas_main.c | 6 ++++++
>> drivers/scsi/libsas/sas_init.c | 31 +++++++++++++++++++++++++++++++
>> include/scsi/libsas.h | 1 +
>> 3 files changed, 38 insertions(+)
>>
> Ah, here is it now.
> So ignore that part of my previous comment.
>
Hi Hannes,
Thanks for checking this.
Can you please re-review "[PATCH v5 2/7] scsi: libsas: shut down the PHY
if events reached the threshold" to see if you are happy with it? We
have no tag currently.
Very much appreciated,
John
> Reviewed-by: Hannes Reinecke <[email protected]>
>
> Cheers,
>
> Hannes
>
On 12/15/2017 01:18 PM, Hannes Reinecke wrote:
> On 12/08/2017 10:42 AM, Jason Yan wrote:
>> 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 <[email protected]>
>> Signed-off-by: Jason Yan <[email protected]>
>> CC: John Garry <[email protected]>
>> CC: Johannes Thumshirn <[email protected]>
>> CC: Ewan Milne <[email protected]>
>> CC: Christoph Hellwig <[email protected]>
>> CC: Tomas Henzl <[email protected]>
>> ---
>> drivers/scsi/libsas/sas_init.c | 33 ++++++++++++++++++++++++++++++++-
>> drivers/scsi/libsas/sas_phy.c | 27 ++++++++++++++++++++++++++-
>> include/scsi/libsas.h | 6 ++++++
>> 3 files changed, 64 insertions(+), 2 deletions(-)
>>
> Well, this still looks a bit error prone; what if the system runs out of
> memory before the pool is exhausted?
> (Also a threshold of 1024 events is a bit arbitrary; one might want to
> adjust that).
>
> Couldn't you allocate two static events always (for shutdown and signal
> loss), and then use a fixed pool?
>
Has actually been resolved by the next patch.
Reviewed-by: Hannes Reinecke <[email protected]>
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
[email protected] +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)