* [PATCH 01/12] libsas: introduce sas_work to fix sas_drain_work vs sas_queue_work
2012-04-13 23:36 [GIT PATCH 00/12] libsas fixes for 3.4 Dan Williams
@ 2012-04-13 23:36 ` Dan Williams
2012-04-13 23:37 ` [PATCH 02/12] libsas: cleanup spurious calls to scsi_schedule_eh Dan Williams
` (11 subsequent siblings)
12 siblings, 0 replies; 34+ messages in thread
From: Dan Williams @ 2012-04-13 23:36 UTC (permalink / raw)
To: JBottomley; +Cc: linux-ide, linux-scsi
When requeuing work to a draining workqueue the last work instance may
not be idle, so sas_queue_work() must not touch work->entry. Introduce
sas_work with a drain_node list_head to have a private list for
collecting work deferred due to drain collision.
Fixes reports like:
BUG: unable to handle kernel NULL pointer dereference at (null)
IP: [<ffffffff810410d4>] process_one_work+0x2e/0x338
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/scsi/libsas/sas_discover.c | 28 +++++++++++++------------
drivers/scsi/libsas/sas_event.c | 24 ++++++++++++----------
drivers/scsi/libsas/sas_init.c | 11 +++++-----
drivers/scsi/libsas/sas_internal.h | 6 +++--
drivers/scsi/libsas/sas_phy.c | 21 ++++++-------------
drivers/scsi/libsas/sas_port.c | 15 +++++---------
include/scsi/libsas.h | 40 ++++++++++++++++++++++++++++++++----
7 files changed, 83 insertions(+), 62 deletions(-)
diff --git a/drivers/scsi/libsas/sas_discover.c b/drivers/scsi/libsas/sas_discover.c
index 3646796..c7ac882 100644
--- a/drivers/scsi/libsas/sas_discover.c
+++ b/drivers/scsi/libsas/sas_discover.c
@@ -205,8 +205,7 @@ void sas_notify_lldd_dev_gone(struct domain_device *dev)
static void sas_probe_devices(struct work_struct *work)
{
struct domain_device *dev, *n;
- struct sas_discovery_event *ev =
- container_of(work, struct sas_discovery_event, work);
+ struct sas_discovery_event *ev = to_sas_discovery_event(work);
struct asd_sas_port *port = ev->port;
clear_bit(DISCE_PROBE, &port->disc.pending);
@@ -291,8 +290,7 @@ static void sas_unregister_common_dev(struct asd_sas_port *port, struct domain_d
static void sas_destruct_devices(struct work_struct *work)
{
struct domain_device *dev, *n;
- struct sas_discovery_event *ev =
- container_of(work, struct sas_discovery_event, work);
+ struct sas_discovery_event *ev = to_sas_discovery_event(work);
struct asd_sas_port *port = ev->port;
clear_bit(DISCE_DESTRUCT, &port->disc.pending);
@@ -377,8 +375,7 @@ static void sas_discover_domain(struct work_struct *work)
{
struct domain_device *dev;
int error = 0;
- struct sas_discovery_event *ev =
- container_of(work, struct sas_discovery_event, work);
+ struct sas_discovery_event *ev = to_sas_discovery_event(work);
struct asd_sas_port *port = ev->port;
clear_bit(DISCE_DISCOVER_DOMAIN, &port->disc.pending);
@@ -437,8 +434,7 @@ static void sas_discover_domain(struct work_struct *work)
static void sas_revalidate_domain(struct work_struct *work)
{
int res = 0;
- struct sas_discovery_event *ev =
- container_of(work, struct sas_discovery_event, work);
+ struct sas_discovery_event *ev = to_sas_discovery_event(work);
struct asd_sas_port *port = ev->port;
struct sas_ha_struct *ha = port->ha;
@@ -466,21 +462,25 @@ static void sas_revalidate_domain(struct work_struct *work)
/* ---------- Events ---------- */
-static void sas_chain_work(struct sas_ha_struct *ha, struct work_struct *work)
+static void sas_chain_work(struct sas_ha_struct *ha, struct sas_work *sw)
{
- /* chained work is not subject to SA_HA_DRAINING or SAS_HA_REGISTERED */
- scsi_queue_work(ha->core.shost, work);
+ /* chained work is not subject to SA_HA_DRAINING or
+ * SAS_HA_REGISTERED, because it is either submitted in the
+ * workqueue, or known to be submitted from a context that is
+ * not racing against draining
+ */
+ scsi_queue_work(ha->core.shost, &sw->work);
}
static void sas_chain_event(int event, unsigned long *pending,
- struct work_struct *work,
+ struct sas_work *sw,
struct sas_ha_struct *ha)
{
if (!test_and_set_bit(event, pending)) {
unsigned long flags;
spin_lock_irqsave(&ha->state_lock, flags);
- sas_chain_work(ha, work);
+ sas_chain_work(ha, sw);
spin_unlock_irqrestore(&ha->state_lock, flags);
}
}
@@ -519,7 +519,7 @@ void sas_init_disc(struct sas_discovery *disc, struct asd_sas_port *port)
disc->pending = 0;
for (i = 0; i < DISC_NUM_EVENTS; i++) {
- INIT_WORK(&disc->disc_work[i].work, sas_event_fns[i]);
+ INIT_SAS_WORK(&disc->disc_work[i].work, sas_event_fns[i]);
disc->disc_work[i].port = port;
}
}
diff --git a/drivers/scsi/libsas/sas_event.c b/drivers/scsi/libsas/sas_event.c
index 16639bb..4e4292d 100644
--- a/drivers/scsi/libsas/sas_event.c
+++ b/drivers/scsi/libsas/sas_event.c
@@ -27,19 +27,21 @@
#include "sas_internal.h"
#include "sas_dump.h"
-void sas_queue_work(struct sas_ha_struct *ha, struct work_struct *work)
+void sas_queue_work(struct sas_ha_struct *ha, struct sas_work *sw)
{
if (!test_bit(SAS_HA_REGISTERED, &ha->state))
return;
- if (test_bit(SAS_HA_DRAINING, &ha->state))
- list_add(&work->entry, &ha->defer_q);
- else
- scsi_queue_work(ha->core.shost, work);
+ 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);
+ } else
+ scsi_queue_work(ha->core.shost, &sw->work);
}
static void sas_queue_event(int event, unsigned long *pending,
- struct work_struct *work,
+ struct sas_work *work,
struct sas_ha_struct *ha)
{
if (!test_and_set_bit(event, pending)) {
@@ -55,7 +57,7 @@ static void sas_queue_event(int event, unsigned long *pending,
void __sas_drain_work(struct sas_ha_struct *ha)
{
struct workqueue_struct *wq = ha->core.shost->work_q;
- struct work_struct *w, *_w;
+ struct sas_work *sw, *_sw;
set_bit(SAS_HA_DRAINING, &ha->state);
/* flush submitters */
@@ -66,9 +68,9 @@ void __sas_drain_work(struct sas_ha_struct *ha)
spin_lock_irq(&ha->state_lock);
clear_bit(SAS_HA_DRAINING, &ha->state);
- list_for_each_entry_safe(w, _w, &ha->defer_q, entry) {
- list_del_init(&w->entry);
- sas_queue_work(ha, w);
+ list_for_each_entry_safe(sw, _sw, &ha->defer_q, drain_node) {
+ list_del_init(&sw->drain_node);
+ sas_queue_work(ha, sw);
}
spin_unlock_irq(&ha->state_lock);
}
@@ -151,7 +153,7 @@ int sas_init_events(struct sas_ha_struct *sas_ha)
int i;
for (i = 0; i < HA_NUM_EVENTS; i++) {
- INIT_WORK(&sas_ha->ha_events[i].work, sas_ha_event_fns[i]);
+ INIT_SAS_WORK(&sas_ha->ha_events[i].work, sas_ha_event_fns[i]);
sas_ha->ha_events[i].ha = sas_ha;
}
diff --git a/drivers/scsi/libsas/sas_init.c b/drivers/scsi/libsas/sas_init.c
index 120bff6..10cb5ae 100644
--- a/drivers/scsi/libsas/sas_init.c
+++ b/drivers/scsi/libsas/sas_init.c
@@ -94,8 +94,7 @@ void sas_hash_addr(u8 *hashed, const u8 *sas_addr)
void sas_hae_reset(struct work_struct *work)
{
- struct sas_ha_event *ev =
- container_of(work, struct sas_ha_event, work);
+ struct sas_ha_event *ev = to_sas_ha_event(work);
struct sas_ha_struct *ha = ev->ha;
clear_bit(HAE_RESET, &ha->pending);
@@ -369,14 +368,14 @@ static void sas_phy_release(struct sas_phy *phy)
static void phy_reset_work(struct work_struct *work)
{
- struct sas_phy_data *d = container_of(work, typeof(*d), reset_work);
+ struct sas_phy_data *d = container_of(work, typeof(*d), reset_work.work);
d->reset_result = transport_sas_phy_reset(d->phy, d->hard_reset);
}
static void phy_enable_work(struct work_struct *work)
{
- struct sas_phy_data *d = container_of(work, typeof(*d), enable_work);
+ struct sas_phy_data *d = container_of(work, typeof(*d), enable_work.work);
d->enable_result = sas_phy_enable(d->phy, d->enable);
}
@@ -389,8 +388,8 @@ static int sas_phy_setup(struct sas_phy *phy)
return -ENOMEM;
mutex_init(&d->event_lock);
- INIT_WORK(&d->reset_work, phy_reset_work);
- INIT_WORK(&d->enable_work, phy_enable_work);
+ INIT_SAS_WORK(&d->reset_work, phy_reset_work);
+ INIT_SAS_WORK(&d->enable_work, phy_enable_work);
d->phy = phy;
phy->hostdata = d;
diff --git a/drivers/scsi/libsas/sas_internal.h b/drivers/scsi/libsas/sas_internal.h
index f05c638..507e4cf 100644
--- a/drivers/scsi/libsas/sas_internal.h
+++ b/drivers/scsi/libsas/sas_internal.h
@@ -45,10 +45,10 @@ struct sas_phy_data {
struct mutex event_lock;
int hard_reset;
int reset_result;
- struct work_struct reset_work;
+ struct sas_work reset_work;
int enable;
int enable_result;
- struct work_struct enable_work;
+ struct sas_work enable_work;
};
void sas_scsi_recover_host(struct Scsi_Host *shost);
@@ -80,7 +80,7 @@ void sas_porte_broadcast_rcvd(struct work_struct *work);
void sas_porte_link_reset_err(struct work_struct *work);
void sas_porte_timer_event(struct work_struct *work);
void sas_porte_hard_reset(struct work_struct *work);
-void sas_queue_work(struct sas_ha_struct *ha, struct work_struct *work);
+void sas_queue_work(struct sas_ha_struct *ha, struct sas_work *sw);
int sas_notify_lldd_dev_found(struct domain_device *);
void sas_notify_lldd_dev_gone(struct domain_device *);
diff --git a/drivers/scsi/libsas/sas_phy.c b/drivers/scsi/libsas/sas_phy.c
index dcfd4a9..521422e 100644
--- a/drivers/scsi/libsas/sas_phy.c
+++ b/drivers/scsi/libsas/sas_phy.c
@@ -32,8 +32,7 @@
static void sas_phye_loss_of_signal(struct work_struct *work)
{
- struct asd_sas_event *ev =
- container_of(work, struct asd_sas_event, 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);
@@ -43,8 +42,7 @@ static void sas_phye_loss_of_signal(struct work_struct *work)
static void sas_phye_oob_done(struct work_struct *work)
{
- struct asd_sas_event *ev =
- container_of(work, struct asd_sas_event, 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);
@@ -53,8 +51,7 @@ static void sas_phye_oob_done(struct work_struct *work)
static void sas_phye_oob_error(struct work_struct *work)
{
- struct asd_sas_event *ev =
- container_of(work, struct asd_sas_event, 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 asd_sas_port *port = phy->port;
@@ -85,8 +82,7 @@ static void sas_phye_oob_error(struct work_struct *work)
static void sas_phye_spinup_hold(struct work_struct *work)
{
- struct asd_sas_event *ev =
- container_of(work, struct asd_sas_event, 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 =
@@ -127,14 +123,12 @@ int sas_register_phys(struct sas_ha_struct *sas_ha)
phy->error = 0;
INIT_LIST_HEAD(&phy->port_phy_el);
for (k = 0; k < PORT_NUM_EVENTS; k++) {
- INIT_WORK(&phy->port_events[k].work,
- sas_port_event_fns[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_WORK(&phy->phy_events[k].work,
- sas_phy_event_fns[k]);
+ INIT_SAS_WORK(&phy->phy_events[k].work, sas_phy_event_fns[k]);
phy->phy_events[k].phy = phy;
}
@@ -144,8 +138,7 @@ int sas_register_phys(struct sas_ha_struct *sas_ha)
spin_lock_init(&phy->sas_prim_lock);
phy->frame_rcvd_size = 0;
- phy->phy = sas_phy_alloc(&sas_ha->core.shost->shost_gendev,
- i);
+ phy->phy = sas_phy_alloc(&sas_ha->core.shost->shost_gendev, i);
if (!phy->phy)
return -ENOMEM;
diff --git a/drivers/scsi/libsas/sas_port.c b/drivers/scsi/libsas/sas_port.c
index eb19c01..1cf7d75 100644
--- a/drivers/scsi/libsas/sas_port.c
+++ b/drivers/scsi/libsas/sas_port.c
@@ -208,8 +208,7 @@ void sas_deform_port(struct asd_sas_phy *phy, int gone)
void sas_porte_bytes_dmaed(struct work_struct *work)
{
- struct asd_sas_event *ev =
- container_of(work, struct asd_sas_event, 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);
@@ -219,8 +218,7 @@ void sas_porte_bytes_dmaed(struct work_struct *work)
void sas_porte_broadcast_rcvd(struct work_struct *work)
{
- struct asd_sas_event *ev =
- container_of(work, struct asd_sas_event, work);
+ struct asd_sas_event *ev = to_asd_sas_event(work);
struct asd_sas_phy *phy = ev->phy;
unsigned long flags;
u32 prim;
@@ -237,8 +235,7 @@ void sas_porte_broadcast_rcvd(struct work_struct *work)
void sas_porte_link_reset_err(struct work_struct *work)
{
- struct asd_sas_event *ev =
- container_of(work, struct asd_sas_event, 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);
@@ -248,8 +245,7 @@ void sas_porte_link_reset_err(struct work_struct *work)
void sas_porte_timer_event(struct work_struct *work)
{
- struct asd_sas_event *ev =
- container_of(work, struct asd_sas_event, 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);
@@ -259,8 +255,7 @@ void sas_porte_timer_event(struct work_struct *work)
void sas_porte_hard_reset(struct work_struct *work)
{
- struct asd_sas_event *ev =
- container_of(work, struct asd_sas_event, 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);
diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index 5f5ed1b..f4f1c96 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -217,11 +217,29 @@ struct domain_device {
struct kref kref;
};
-struct sas_discovery_event {
+struct sas_work {
+ struct list_head drain_node;
struct work_struct work;
+};
+
+static inline void INIT_SAS_WORK(struct sas_work *sw, void (*fn)(struct work_struct *))
+{
+ INIT_WORK(&sw->work, fn);
+ INIT_LIST_HEAD(&sw->drain_node);
+}
+
+struct sas_discovery_event {
+ struct sas_work work;
struct asd_sas_port *port;
};
+static inline struct sas_discovery_event *to_sas_discovery_event(struct work_struct *work)
+{
+ struct sas_discovery_event *ev = container_of(work, typeof(*ev), work.work);
+
+ return ev;
+}
+
struct sas_discovery {
struct sas_discovery_event disc_work[DISC_NUM_EVENTS];
unsigned long pending;
@@ -244,7 +262,7 @@ struct asd_sas_port {
struct list_head destroy_list;
enum sas_linkrate linkrate;
- struct work_struct work;
+ struct sas_work work;
/* public: */
int id;
@@ -270,10 +288,17 @@ struct asd_sas_port {
};
struct asd_sas_event {
- struct work_struct work;
+ struct sas_work work;
struct asd_sas_phy *phy;
};
+static inline struct asd_sas_event *to_asd_sas_event(struct work_struct *work)
+{
+ struct asd_sas_event *ev = container_of(work, typeof(*ev), work.work);
+
+ return ev;
+}
+
/* The phy pretty much is controlled by the LLDD.
* The class only reads those fields.
*/
@@ -333,10 +358,17 @@ struct scsi_core {
};
struct sas_ha_event {
- struct work_struct work;
+ 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,
^ permalink raw reply related [flat|nested] 34+ messages in thread* [PATCH 02/12] libsas: cleanup spurious calls to scsi_schedule_eh
2012-04-13 23:36 [GIT PATCH 00/12] libsas fixes for 3.4 Dan Williams
2012-04-13 23:36 ` [PATCH 01/12] libsas: introduce sas_work to fix sas_drain_work vs sas_queue_work Dan Williams
@ 2012-04-13 23:37 ` Dan Williams
2012-04-13 23:37 ` [PATCH 03/12] libata, libsas: introduce sched_eh and end_eh port ops Dan Williams
` (10 subsequent siblings)
12 siblings, 0 replies; 34+ messages in thread
From: Dan Williams @ 2012-04-13 23:37 UTC (permalink / raw)
To: JBottomley
Cc: Maciej Trela, linux-ide, Artur Wojcik, linux-scsi, Jacek Danecki
From: Maciej Trela <maciej.trela@intel.com>
eh is woken up automatically by the presence of failed commands,
scsi_schedule_eh is reserved for cases where there are no failed
commands. This guarantees that host_eh_sceduled is only incremented
when an explicit eh request is made.
Reviewed-by: Jacek Danecki <jacek.danecki@intel.com>
Signed-off-by: Maciej Trela <maciej.trela@intel.com>
[fixed spurious delete of sas_ata_task_abort]
Signed-off-by: Artur Wojcik <artur.wojcik@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/scsi/libsas/sas_ata.c | 1 -
drivers/scsi/libsas/sas_scsi_host.c | 1 -
2 files changed, 2 deletions(-)
diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index bc0cecc..bc83704 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -587,7 +587,6 @@ void sas_ata_task_abort(struct sas_task *task)
spin_lock_irqsave(q->queue_lock, flags);
blk_abort_request(qc->scsicmd->request);
spin_unlock_irqrestore(q->queue_lock, flags);
- scsi_schedule_eh(qc->scsicmd->device->host);
return;
}
diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c
index f0b9b7b..17339e5 100644
--- a/drivers/scsi/libsas/sas_scsi_host.c
+++ b/drivers/scsi/libsas/sas_scsi_host.c
@@ -1003,7 +1003,6 @@ void sas_task_abort(struct sas_task *task)
spin_lock_irqsave(q->queue_lock, flags);
blk_abort_request(sc->request);
spin_unlock_irqrestore(q->queue_lock, flags);
- scsi_schedule_eh(sc->device->host);
}
}
^ permalink raw reply related [flat|nested] 34+ messages in thread* [PATCH 03/12] libata, libsas: introduce sched_eh and end_eh port ops
2012-04-13 23:36 [GIT PATCH 00/12] libsas fixes for 3.4 Dan Williams
2012-04-13 23:36 ` [PATCH 01/12] libsas: introduce sas_work to fix sas_drain_work vs sas_queue_work Dan Williams
2012-04-13 23:37 ` [PATCH 02/12] libsas: cleanup spurious calls to scsi_schedule_eh Dan Williams
@ 2012-04-13 23:37 ` Dan Williams
2012-04-21 6:19 ` Jeff Garzik
2012-04-22 17:30 ` James Bottomley
2012-04-13 23:37 ` [PATCH 04/12] libsas: fix sas_find_bcast_phy() in the presence of 'vacant' phys Dan Williams
` (9 subsequent siblings)
12 siblings, 2 replies; 34+ messages in thread
From: Dan Williams @ 2012-04-13 23:37 UTC (permalink / raw)
To: JBottomley; +Cc: Tejun Heo, linux-ide, linux-scsi, Jacek Danecki
When managing shost->host_eh_scheduled libata assumes that there is a
1:1 shost-to-ata_port relationship. libsas creates a 1:N relationship
so it needs to manage host_eh_scheduled cumulatively at the host level.
The sched_eh and end_eh port port ops allow libsas to track when domain
devices enter/leave the "eh-pending" state under ha->lock (previously
named ha->state_lock, but it is no longer just a lock for ha->state
changes).
Since host_eh_scheduled indicates eh without backing commands pinning
the device it can be deallocated at any time. Move the taking of the
domain_device reference under the port_lock to guarantee that the
ata_port stays around for the duration of eh.
Cc: Tejun Heo <tj@kernel.org>
Acked-by: Jacek Danecki <jacek.danecki@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/ata/libata-core.c | 4 ++
drivers/ata/libata-eh.c | 57 ++++++++++++++++++++++++++++-------
drivers/scsi/libsas/sas_ata.c | 38 +++++++++++++++++++++--
drivers/scsi/libsas/sas_discover.c | 6 ++--
drivers/scsi/libsas/sas_event.c | 12 ++++---
drivers/scsi/libsas/sas_init.c | 14 ++++-----
drivers/scsi/libsas/sas_scsi_host.c | 27 +++++++++++++----
include/linux/libata.h | 4 ++
include/scsi/libsas.h | 4 ++
include/scsi/sas_ata.h | 5 +++
10 files changed, 134 insertions(+), 37 deletions(-)
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index e0bda9f..ea8444e 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -80,6 +80,8 @@ const struct ata_port_operations ata_base_port_ops = {
.prereset = ata_std_prereset,
.postreset = ata_std_postreset,
.error_handler = ata_std_error_handler,
+ .sched_eh = ata_std_sched_eh,
+ .end_eh = ata_std_end_eh,
};
const struct ata_port_operations sata_port_ops = {
@@ -6635,6 +6637,8 @@ struct ata_port_operations ata_dummy_port_ops = {
.qc_prep = ata_noop_qc_prep,
.qc_issue = ata_dummy_qc_issue,
.error_handler = ata_dummy_error_handler,
+ .sched_eh = ata_std_sched_eh,
+ .end_eh = ata_std_end_eh,
};
const struct ata_port_info ata_dummy_port_info = {
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index c61316e..4f12f63 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -793,12 +793,12 @@ void ata_scsi_port_error_handler(struct Scsi_Host *host, struct ata_port *ap)
ata_for_each_link(link, ap, HOST_FIRST)
memset(&link->eh_info, 0, sizeof(link->eh_info));
- /* Clear host_eh_scheduled while holding ap->lock such
- * that if exception occurs after this point but
- * before EH completion, SCSI midlayer will
+ /* end eh (clear host_eh_scheduled) while holding
+ * ap->lock such that if exception occurs after this
+ * point but before EH completion, SCSI midlayer will
* re-initiate EH.
*/
- host->host_eh_scheduled = 0;
+ ap->ops->end_eh(ap);
spin_unlock_irqrestore(ap->lock, flags);
ata_eh_release(ap);
@@ -986,16 +986,13 @@ void ata_qc_schedule_eh(struct ata_queued_cmd *qc)
}
/**
- * ata_port_schedule_eh - schedule error handling without a qc
- * @ap: ATA port to schedule EH for
- *
- * Schedule error handling for @ap. EH will kick in as soon as
- * all commands are drained.
+ * ata_std_sched_eh - non-libsas ata_ports issue eh with this common routine
+ * @ap: ATA port to schedule EH for
*
- * LOCKING:
+ * LOCKING: inherited from ata_port_schedule_eh
* spin_lock_irqsave(host lock)
*/
-void ata_port_schedule_eh(struct ata_port *ap)
+void ata_std_sched_eh(struct ata_port *ap)
{
WARN_ON(!ap->ops->error_handler);
@@ -1007,6 +1004,44 @@ void ata_port_schedule_eh(struct ata_port *ap)
DPRINTK("port EH scheduled\n");
}
+EXPORT_SYMBOL_GPL(ata_std_sched_eh);
+
+/**
+ * ata_std_end_eh - non-libsas ata_ports complete eh with this common routine
+ * @ap: ATA port to end EH for
+ *
+ * In the libata object model there is a 1:1 mapping of ata_port to
+ * shost, so host fields can be directly manipulated under ap->lock, in
+ * the libsas case we need to hold a lock at the ha->level to coordinate
+ * these events.
+ *
+ * LOCKING:
+ * spin_lock_irqsave(host lock)
+ */
+void ata_std_end_eh(struct ata_port *ap)
+{
+ struct Scsi_Host *host = ap->scsi_host;
+
+ host->host_eh_scheduled = 0;
+}
+EXPORT_SYMBOL(ata_std_end_eh);
+
+
+/**
+ * ata_port_schedule_eh - schedule error handling without a qc
+ * @ap: ATA port to schedule EH for
+ *
+ * Schedule error handling for @ap. EH will kick in as soon as
+ * all commands are drained.
+ *
+ * LOCKING:
+ * spin_lock_irqsave(host lock)
+ */
+void ata_port_schedule_eh(struct ata_port *ap)
+{
+ /* see: ata_std_sched_eh, unless you know better */
+ ap->ops->sched_eh(ap);
+}
static int ata_do_link_abort(struct ata_port *ap, struct ata_link *link)
{
diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index bc83704..545b78d 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -523,6 +523,31 @@ static void sas_ata_set_dmamode(struct ata_port *ap, struct ata_device *ata_dev)
i->dft->lldd_ata_set_dmamode(dev);
}
+static void sas_ata_sched_eh(struct ata_port *ap)
+{
+ struct domain_device *dev = ap->private_data;
+ struct sas_ha_struct *ha = dev->port->ha;
+ unsigned long flags;
+
+ spin_lock_irqsave(&ha->lock, flags);
+ if (!test_and_set_bit(SAS_DEV_EH_PENDING, &dev->state))
+ ha->eh_active++;
+ ata_std_sched_eh(ap);
+ spin_unlock_irqrestore(&ha->lock, flags);
+}
+
+void sas_ata_end_eh(struct ata_port *ap)
+{
+ struct domain_device *dev = ap->private_data;
+ struct sas_ha_struct *ha = dev->port->ha;
+ unsigned long flags;
+
+ spin_lock_irqsave(&ha->lock, flags);
+ if (test_and_clear_bit(SAS_DEV_EH_PENDING, &dev->state))
+ ha->eh_active--;
+ spin_unlock_irqrestore(&ha->lock, flags);
+}
+
static struct ata_port_operations sas_sata_ops = {
.prereset = ata_std_prereset,
.hardreset = sas_ata_hard_reset,
@@ -536,6 +561,8 @@ static struct ata_port_operations sas_sata_ops = {
.port_start = ata_sas_port_start,
.port_stop = ata_sas_port_stop,
.set_dmamode = sas_ata_set_dmamode,
+ .sched_eh = sas_ata_sched_eh,
+ .end_eh = sas_ata_end_eh,
};
static struct ata_port_info sata_port_info = {
@@ -708,10 +735,6 @@ static void async_sas_ata_eh(void *data, async_cookie_t cookie)
struct ata_port *ap = dev->sata_dev.ap;
struct sas_ha_struct *ha = dev->port->ha;
- /* hold a reference over eh since we may be racing with final
- * remove once all commands are completed
- */
- kref_get(&dev->kref);
sas_ata_printk(KERN_DEBUG, dev, "dev error handler\n");
ata_scsi_port_error_handler(ha->core.shost, ap);
sas_put_device(dev);
@@ -754,6 +777,13 @@ void sas_ata_strategy_handler(struct Scsi_Host *shost)
list_for_each_entry(dev, &port->dev_list, dev_list_node) {
if (!sas_ata_dev_eh_valid(dev))
continue;
+
+ /* hold a reference over eh since we may be
+ * racing with final remove once all commands
+ * are completed
+ */
+ kref_get(&dev->kref);
+
async_schedule_domain(async_sas_ata_eh, dev, &async);
}
spin_unlock(&port->dev_list_lock);
diff --git a/drivers/scsi/libsas/sas_discover.c b/drivers/scsi/libsas/sas_discover.c
index c7ac882..b82949c 100644
--- a/drivers/scsi/libsas/sas_discover.c
+++ b/drivers/scsi/libsas/sas_discover.c
@@ -282,6 +282,8 @@ static void sas_unregister_common_dev(struct asd_sas_port *port, struct domain_d
spin_lock_irq(&port->dev_list_lock);
list_del_init(&dev->dev_list_node);
+ if (dev_is_sata(dev))
+ sas_ata_end_eh(dev->sata_dev.ap);
spin_unlock_irq(&port->dev_list_lock);
sas_put_device(dev);
@@ -479,9 +481,9 @@ static void sas_chain_event(int event, unsigned long *pending,
if (!test_and_set_bit(event, pending)) {
unsigned long flags;
- spin_lock_irqsave(&ha->state_lock, flags);
+ spin_lock_irqsave(&ha->lock, flags);
sas_chain_work(ha, sw);
- spin_unlock_irqrestore(&ha->state_lock, flags);
+ spin_unlock_irqrestore(&ha->lock, flags);
}
}
diff --git a/drivers/scsi/libsas/sas_event.c b/drivers/scsi/libsas/sas_event.c
index 4e4292d..789c4d8 100644
--- a/drivers/scsi/libsas/sas_event.c
+++ b/drivers/scsi/libsas/sas_event.c
@@ -47,9 +47,9 @@ static void sas_queue_event(int event, unsigned long *pending,
if (!test_and_set_bit(event, pending)) {
unsigned long flags;
- spin_lock_irqsave(&ha->state_lock, flags);
+ spin_lock_irqsave(&ha->lock, flags);
sas_queue_work(ha, work);
- spin_unlock_irqrestore(&ha->state_lock, flags);
+ spin_unlock_irqrestore(&ha->lock, flags);
}
}
@@ -61,18 +61,18 @@ void __sas_drain_work(struct sas_ha_struct *ha)
set_bit(SAS_HA_DRAINING, &ha->state);
/* flush submitters */
- spin_lock_irq(&ha->state_lock);
- spin_unlock_irq(&ha->state_lock);
+ spin_lock_irq(&ha->lock);
+ spin_unlock_irq(&ha->lock);
drain_workqueue(wq);
- spin_lock_irq(&ha->state_lock);
+ spin_lock_irq(&ha->lock);
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);
}
- spin_unlock_irq(&ha->state_lock);
+ spin_unlock_irq(&ha->lock);
}
int sas_drain_work(struct sas_ha_struct *ha)
diff --git a/drivers/scsi/libsas/sas_init.c b/drivers/scsi/libsas/sas_init.c
index 10cb5ae..6909fef 100644
--- a/drivers/scsi/libsas/sas_init.c
+++ b/drivers/scsi/libsas/sas_init.c
@@ -114,7 +114,7 @@ int sas_register_ha(struct sas_ha_struct *sas_ha)
sas_ha->lldd_queue_size = 128; /* Sanity */
set_bit(SAS_HA_REGISTERED, &sas_ha->state);
- spin_lock_init(&sas_ha->state_lock);
+ spin_lock_init(&sas_ha->lock);
mutex_init(&sas_ha->drain_mutex);
INIT_LIST_HEAD(&sas_ha->defer_q);
@@ -163,9 +163,9 @@ int sas_unregister_ha(struct sas_ha_struct *sas_ha)
* events to be queued, and flush any in-progress drainers
*/
mutex_lock(&sas_ha->drain_mutex);
- spin_lock_irq(&sas_ha->state_lock);
+ spin_lock_irq(&sas_ha->lock);
clear_bit(SAS_HA_REGISTERED, &sas_ha->state);
- spin_unlock_irq(&sas_ha->state_lock);
+ spin_unlock_irq(&sas_ha->lock);
__sas_drain_work(sas_ha);
mutex_unlock(&sas_ha->drain_mutex);
@@ -411,9 +411,9 @@ static int queue_phy_reset(struct sas_phy *phy, int hard_reset)
d->reset_result = 0;
d->hard_reset = hard_reset;
- spin_lock_irq(&ha->state_lock);
+ spin_lock_irq(&ha->lock);
sas_queue_work(ha, &d->reset_work);
- spin_unlock_irq(&ha->state_lock);
+ spin_unlock_irq(&ha->lock);
rc = sas_drain_work(ha);
if (rc == 0)
@@ -438,9 +438,9 @@ static int queue_phy_enable(struct sas_phy *phy, int enable)
d->enable_result = 0;
d->enable = enable;
- spin_lock_irq(&ha->state_lock);
+ spin_lock_irq(&ha->lock);
sas_queue_work(ha, &d->enable_work);
- spin_unlock_irq(&ha->state_lock);
+ spin_unlock_irq(&ha->lock);
rc = sas_drain_work(ha);
if (rc == 0)
diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c
index 17339e5..52d5b01 100644
--- a/drivers/scsi/libsas/sas_scsi_host.c
+++ b/drivers/scsi/libsas/sas_scsi_host.c
@@ -667,16 +667,20 @@ static void sas_eh_handle_sas_errors(struct Scsi_Host *shost, struct list_head *
goto out;
}
+
void sas_scsi_recover_host(struct Scsi_Host *shost)
{
struct sas_ha_struct *ha = SHOST_TO_SAS_HA(shost);
- unsigned long flags;
LIST_HEAD(eh_work_q);
+ int tries = 0;
+ bool retry;
- spin_lock_irqsave(shost->host_lock, flags);
+retry:
+ tries++;
+ retry = true;
+ spin_lock_irq(shost->host_lock);
list_splice_init(&shost->eh_cmd_q, &eh_work_q);
- shost->host_eh_scheduled = 0;
- spin_unlock_irqrestore(shost->host_lock, flags);
+ spin_unlock_irq(shost->host_lock);
SAS_DPRINTK("Enter %s busy: %d failed: %d\n",
__func__, shost->host_busy, shost->host_failed);
@@ -710,8 +714,19 @@ out:
scsi_eh_flush_done_q(&ha->eh_done_q);
- SAS_DPRINTK("--- Exit %s: busy: %d failed: %d\n",
- __func__, shost->host_busy, shost->host_failed);
+ /* check if any new eh work was scheduled during the last run */
+ spin_lock_irq(&ha->lock);
+ if (ha->eh_active == 0) {
+ shost->host_eh_scheduled = 0;
+ retry = false;
+ }
+ spin_unlock_irq(&ha->lock);
+
+ if (retry)
+ goto retry;
+
+ SAS_DPRINTK("--- Exit %s: busy: %d failed: %d tries: %d\n",
+ __func__, shost->host_busy, shost->host_failed, tries);
}
enum blk_eh_timer_return sas_scsi_timed_out(struct scsi_cmnd *cmd)
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 42378d6..83ff256 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -845,6 +845,8 @@ struct ata_port_operations {
void (*error_handler)(struct ata_port *ap);
void (*lost_interrupt)(struct ata_port *ap);
void (*post_internal_cmd)(struct ata_queued_cmd *qc);
+ void (*sched_eh)(struct ata_port *ap);
+ void (*end_eh)(struct ata_port *ap);
/*
* Optional features
@@ -1165,6 +1167,8 @@ extern void ata_do_eh(struct ata_port *ap, ata_prereset_fn_t prereset,
ata_reset_fn_t softreset, ata_reset_fn_t hardreset,
ata_postreset_fn_t postreset);
extern void ata_std_error_handler(struct ata_port *ap);
+extern void ata_std_sched_eh(struct ata_port *ap);
+extern void ata_std_end_eh(struct ata_port *ap);
extern int ata_link_nr_enabled(struct ata_link *link);
/*
diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index f4f1c96..2718b24 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -177,6 +177,7 @@ struct sata_device {
enum {
SAS_DEV_GONE,
SAS_DEV_DESTROY,
+ SAS_DEV_EH_PENDING,
};
struct domain_device {
@@ -384,7 +385,8 @@ struct sas_ha_struct {
struct list_head defer_q; /* work queued while draining */
struct mutex drain_mutex;
unsigned long state;
- spinlock_t state_lock;
+ spinlock_t lock;
+ int eh_active;
struct mutex disco_mutex;
diff --git a/include/scsi/sas_ata.h b/include/scsi/sas_ata.h
index cdccd2e..cae55b6 100644
--- a/include/scsi/sas_ata.h
+++ b/include/scsi/sas_ata.h
@@ -45,6 +45,7 @@ void sas_ata_eh(struct Scsi_Host *shost, struct list_head *work_q,
void sas_ata_schedule_reset(struct domain_device *dev);
void sas_ata_wait_eh(struct domain_device *dev);
void sas_probe_sata(struct asd_sas_port *port);
+void sas_ata_end_eh(struct ata_port *ap);
#else
@@ -85,6 +86,10 @@ static inline int sas_get_ata_info(struct domain_device *dev, struct ex_phy *phy
{
return 0;
}
+
+static inline void sas_ata_end_eh(struct ata_port *ap)
+{
+}
#endif
#endif /* _SAS_ATA_H_ */
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH 03/12] libata, libsas: introduce sched_eh and end_eh port ops
2012-04-13 23:37 ` [PATCH 03/12] libata, libsas: introduce sched_eh and end_eh port ops Dan Williams
@ 2012-04-21 6:19 ` Jeff Garzik
2012-04-22 17:30 ` James Bottomley
1 sibling, 0 replies; 34+ messages in thread
From: Jeff Garzik @ 2012-04-21 6:19 UTC (permalink / raw)
To: Dan Williams; +Cc: JBottomley, Tejun Heo, linux-ide, linux-scsi, Jacek Danecki
On 04/13/2012 07:37 PM, Dan Williams wrote:
> When managing shost->host_eh_scheduled libata assumes that there is a
> 1:1 shost-to-ata_port relationship. libsas creates a 1:N relationship
> so it needs to manage host_eh_scheduled cumulatively at the host level.
> The sched_eh and end_eh port port ops allow libsas to track when domain
> devices enter/leave the "eh-pending" state under ha->lock (previously
> named ha->state_lock, but it is no longer just a lock for ha->state
> changes).
>
> Since host_eh_scheduled indicates eh without backing commands pinning
> the device it can be deallocated at any time. Move the taking of the
> domain_device reference under the port_lock to guarantee that the
> ata_port stays around for the duration of eh.
>
> Cc: Tejun Heo<tj@kernel.org>
> Acked-by: Jacek Danecki<jacek.danecki@intel.com>
> Signed-off-by: Dan Williams<dan.j.williams@intel.com>
> ---
> drivers/ata/libata-core.c | 4 ++
> drivers/ata/libata-eh.c | 57 ++++++++++++++++++++++++++++-------
> drivers/scsi/libsas/sas_ata.c | 38 +++++++++++++++++++++--
> drivers/scsi/libsas/sas_discover.c | 6 ++--
> drivers/scsi/libsas/sas_event.c | 12 ++++---
> drivers/scsi/libsas/sas_init.c | 14 ++++-----
> drivers/scsi/libsas/sas_scsi_host.c | 27 +++++++++++++----
> include/linux/libata.h | 4 ++
> include/scsi/libsas.h | 4 ++
> include/scsi/sas_ata.h | 5 +++
> 10 files changed, 134 insertions(+), 37 deletions(-)
Acked-by: Jeff Garzik <jgarzik@redhat.com>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 03/12] libata, libsas: introduce sched_eh and end_eh port ops
2012-04-13 23:37 ` [PATCH 03/12] libata, libsas: introduce sched_eh and end_eh port ops Dan Williams
2012-04-21 6:19 ` Jeff Garzik
@ 2012-04-22 17:30 ` James Bottomley
2012-04-23 2:33 ` Jeff Garzik
2012-04-23 19:41 ` Dan Williams
1 sibling, 2 replies; 34+ messages in thread
From: James Bottomley @ 2012-04-22 17:30 UTC (permalink / raw)
To: Dan Williams; +Cc: Tejun Heo, linux-ide, linux-scsi, Jacek Danecki
On Fri, 2012-04-13 at 16:37 -0700, Dan Williams wrote:
> When managing shost->host_eh_scheduled libata assumes that there is a
> 1:1 shost-to-ata_port relationship. libsas creates a 1:N relationship
> so it needs to manage host_eh_scheduled cumulatively at the host level.
> The sched_eh and end_eh port port ops allow libsas to track when domain
> devices enter/leave the "eh-pending" state under ha->lock (previously
> named ha->state_lock, but it is no longer just a lock for ha->state
> changes).
>
> Since host_eh_scheduled indicates eh without backing commands pinning
> the device it can be deallocated at any time. Move the taking of the
> domain_device reference under the port_lock to guarantee that the
> ata_port stays around for the duration of eh.
> Cc: Tejun Heo <tj@kernel.org>
> Acked-by: Jacek Danecki <jacek.danecki@intel.com>
Could we standardise on Acked-by, please. In my book it means the
maintainer of a piece of code agrees with the change and lets me take it
through my tree. I'm aware that not everyone uses this definition, so
we can use a different standard from my current one, but what does it
mean in this case?
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
> drivers/ata/libata-core.c | 4 ++
> drivers/ata/libata-eh.c | 57 ++++++++++++++++++++++++++++-------
> drivers/scsi/libsas/sas_ata.c | 38 +++++++++++++++++++++--
> drivers/scsi/libsas/sas_discover.c | 6 ++--
> drivers/scsi/libsas/sas_event.c | 12 ++++---
> drivers/scsi/libsas/sas_init.c | 14 ++++-----
> drivers/scsi/libsas/sas_scsi_host.c | 27 +++++++++++++----
> include/linux/libata.h | 4 ++
> include/scsi/libsas.h | 4 ++
> include/scsi/sas_ata.h | 5 +++
> 10 files changed, 134 insertions(+), 37 deletions(-)
This is a pretty big change for rc fixes. None of the other changes in
the series seem to be dependent on it, what bug is it actually fixing?
James
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 03/12] libata, libsas: introduce sched_eh and end_eh port ops
2012-04-22 17:30 ` James Bottomley
@ 2012-04-23 2:33 ` Jeff Garzik
2012-04-23 8:10 ` James Bottomley
2012-04-23 19:41 ` Dan Williams
1 sibling, 1 reply; 34+ messages in thread
From: Jeff Garzik @ 2012-04-23 2:33 UTC (permalink / raw)
To: James Bottomley
Cc: Dan Williams, Tejun Heo, linux-ide, linux-scsi, Jacek Danecki
On 04/22/2012 01:30 PM, James Bottomley wrote:
> On Fri, 2012-04-13 at 16:37 -0700, Dan Williams wrote:
>> When managing shost->host_eh_scheduled libata assumes that there is a
>> 1:1 shost-to-ata_port relationship. libsas creates a 1:N relationship
>> so it needs to manage host_eh_scheduled cumulatively at the host level.
>> The sched_eh and end_eh port port ops allow libsas to track when domain
>> devices enter/leave the "eh-pending" state under ha->lock (previously
>> named ha->state_lock, but it is no longer just a lock for ha->state
>> changes).
>>
>> Since host_eh_scheduled indicates eh without backing commands pinning
>> the device it can be deallocated at any time. Move the taking of the
>> domain_device reference under the port_lock to guarantee that the
>> ata_port stays around for the duration of eh.
>
>> Cc: Tejun Heo<tj@kernel.org>
>> Acked-by: Jacek Danecki<jacek.danecki@intel.com>
>
> Could we standardise on Acked-by, please. In my book it means the
> maintainer of a piece of code agrees with the change and lets me take it
> through my tree. I'm aware that not everyone uses this definition, so
> we can use a different standard from my current one, but what does it
> mean in this case?
The above, IMO, should be s/Acked-by/Signed-off-by/
FWIW this also has
Acked-by: Jeff Garzik <jgarzik@redhat.com>
as noted days ago in another thread.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 03/12] libata, libsas: introduce sched_eh and end_eh port ops
2012-04-23 2:33 ` Jeff Garzik
@ 2012-04-23 8:10 ` James Bottomley
2012-04-23 19:13 ` Dan Williams
0 siblings, 1 reply; 34+ messages in thread
From: James Bottomley @ 2012-04-23 8:10 UTC (permalink / raw)
To: Jeff Garzik; +Cc: Dan Williams, Tejun Heo, linux-ide, linux-scsi, Jacek Danecki
On Sun, 2012-04-22 at 22:33 -0400, Jeff Garzik wrote:
> On 04/22/2012 01:30 PM, James Bottomley wrote:
> > On Fri, 2012-04-13 at 16:37 -0700, Dan Williams wrote:
> >> When managing shost->host_eh_scheduled libata assumes that there is a
> >> 1:1 shost-to-ata_port relationship. libsas creates a 1:N relationship
> >> so it needs to manage host_eh_scheduled cumulatively at the host level.
> >> The sched_eh and end_eh port port ops allow libsas to track when domain
> >> devices enter/leave the "eh-pending" state under ha->lock (previously
> >> named ha->state_lock, but it is no longer just a lock for ha->state
> >> changes).
> >>
> >> Since host_eh_scheduled indicates eh without backing commands pinning
> >> the device it can be deallocated at any time. Move the taking of the
> >> domain_device reference under the port_lock to guarantee that the
> >> ata_port stays around for the duration of eh.
> >
> >> Cc: Tejun Heo<tj@kernel.org>
> >> Acked-by: Jacek Danecki<jacek.danecki@intel.com>
> >
> > Could we standardise on Acked-by, please. In my book it means the
> > maintainer of a piece of code agrees with the change and lets me take it
> > through my tree. I'm aware that not everyone uses this definition, so
> > we can use a different standard from my current one, but what does it
> > mean in this case?
>
> The above, IMO, should be s/Acked-by/Signed-off-by/
Yes, I suspect this too.
> FWIW this also has
>
> Acked-by: Jeff Garzik <jgarzik@redhat.com>
>
> as noted days ago in another thread.
Understood, will add.
James
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 03/12] libata, libsas: introduce sched_eh and end_eh port ops
2012-04-23 8:10 ` James Bottomley
@ 2012-04-23 19:13 ` Dan Williams
2012-04-23 22:22 ` James Bottomley
0 siblings, 1 reply; 34+ messages in thread
From: Dan Williams @ 2012-04-23 19:13 UTC (permalink / raw)
To: James Bottomley
Cc: Jeff Garzik, Tejun Heo, linux-ide, linux-scsi, Jacek Danecki
On Mon, Apr 23, 2012 at 1:10 AM, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
> On Sun, 2012-04-22 at 22:33 -0400, Jeff Garzik wrote:
>> On 04/22/2012 01:30 PM, James Bottomley wrote:
>> > On Fri, 2012-04-13 at 16:37 -0700, Dan Williams wrote:
>> >> When managing shost->host_eh_scheduled libata assumes that there is a
>> >> 1:1 shost-to-ata_port relationship. libsas creates a 1:N relationship
>> >> so it needs to manage host_eh_scheduled cumulatively at the host level.
>> >> The sched_eh and end_eh port port ops allow libsas to track when domain
>> >> devices enter/leave the "eh-pending" state under ha->lock (previously
>> >> named ha->state_lock, but it is no longer just a lock for ha->state
>> >> changes).
>> >>
>> >> Since host_eh_scheduled indicates eh without backing commands pinning
>> >> the device it can be deallocated at any time. Move the taking of the
>> >> domain_device reference under the port_lock to guarantee that the
>> >> ata_port stays around for the duration of eh.
>> >
>> >> Cc: Tejun Heo<tj@kernel.org>
>> >> Acked-by: Jacek Danecki<jacek.danecki@intel.com>
>> >
>> > Could we standardise on Acked-by, please. In my book it means the
>> > maintainer of a piece of code agrees with the change and lets me take it
>> > through my tree. I'm aware that not everyone uses this definition, so
>> > we can use a different standard from my current one, but what does it
>> > mean in this case?
>>
>> The above, IMO, should be s/Acked-by/Signed-off-by/
>
> Yes, I suspect this too.
No, it means:
"If a person was not directly involved in the preparation or handling of a
patch but wishes to signify and record their approval of it then they can
arrange to have an Acked-by: line added to the patch's changelog."
"Acked-by: is not as formal as Signed-off-by:. It is a record that the acker
has at least reviewed the patch and has indicated acceptance. Hence patch
mergers will sometimes manually convert an acker's "yep, looks good to me"
into an Acked-by:"
What's wrong with Acked-by? Signed-off-by involves co-authorship or
handled the patch, Reviewed-by is probably better, but maybe not
everyone is comfortable asserting the "Reviewer's statement of
oversight". I'll certainly continue to take Jack's "looks ok to me "
as Acked-by the pm8001 maintainer for libsas changes that don't touch
drivers/scsi/pm8001.
For internal acks we should probably aim for promoting to Reviewed-by
or Tested-by... if Acked-by is unwelcome in scsi.
--
Dan
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 03/12] libata, libsas: introduce sched_eh and end_eh port ops
2012-04-23 19:13 ` Dan Williams
@ 2012-04-23 22:22 ` James Bottomley
2012-04-23 22:49 ` Dan Williams
0 siblings, 1 reply; 34+ messages in thread
From: James Bottomley @ 2012-04-23 22:22 UTC (permalink / raw)
To: Dan Williams; +Cc: Jeff Garzik, Tejun Heo, linux-ide, linux-scsi, Jacek Danecki
On Mon, 2012-04-23 at 12:13 -0700, Dan Williams wrote:
> On Mon, Apr 23, 2012 at 1:10 AM, James Bottomley
> <James.Bottomley@hansenpartnership.com> wrote:
> > On Sun, 2012-04-22 at 22:33 -0400, Jeff Garzik wrote:
> >> On 04/22/2012 01:30 PM, James Bottomley wrote:
> >> > On Fri, 2012-04-13 at 16:37 -0700, Dan Williams wrote:
> >> >> When managing shost->host_eh_scheduled libata assumes that there is a
> >> >> 1:1 shost-to-ata_port relationship. libsas creates a 1:N relationship
> >> >> so it needs to manage host_eh_scheduled cumulatively at the host level.
> >> >> The sched_eh and end_eh port port ops allow libsas to track when domain
> >> >> devices enter/leave the "eh-pending" state under ha->lock (previously
> >> >> named ha->state_lock, but it is no longer just a lock for ha->state
> >> >> changes).
> >> >>
> >> >> Since host_eh_scheduled indicates eh without backing commands pinning
> >> >> the device it can be deallocated at any time. Move the taking of the
> >> >> domain_device reference under the port_lock to guarantee that the
> >> >> ata_port stays around for the duration of eh.
> >> >
> >> >> Cc: Tejun Heo<tj@kernel.org>
> >> >> Acked-by: Jacek Danecki<jacek.danecki@intel.com>
> >> >
> >> > Could we standardise on Acked-by, please. In my book it means the
> >> > maintainer of a piece of code agrees with the change and lets me take it
> >> > through my tree. I'm aware that not everyone uses this definition, so
> >> > we can use a different standard from my current one, but what does it
> >> > mean in this case?
> >>
> >> The above, IMO, should be s/Acked-by/Signed-off-by/
> >
> > Yes, I suspect this too.
>
> No, it means:
>
> "If a person was not directly involved in the preparation or handling of a
> patch but wishes to signify and record their approval of it then they can
> arrange to have an Acked-by: line added to the patch's changelog."
Isn't that tested-by or reviewed-by?
> "Acked-by: is not as formal as Signed-off-by:. It is a record that the acker
> has at least reviewed the patch and has indicated acceptance. Hence patch
> mergers will sometimes manually convert an acker's "yep, looks good to me"
> into an Acked-by:"
Reviewed-by is the appropriate one for that then. Acked-by usually
means the person has more involvement in the particular subsystem than
would be implied by reviewed-by. Like I said, I tend to reserve
acked-by for maintainers of the various elements that have to go through
different trees.
> What's wrong with Acked-by? Signed-off-by involves co-authorship or
> handled the patch, Reviewed-by is probably better, but maybe not
> everyone is comfortable asserting the "Reviewer's statement of
> oversight". I'll certainly continue to take Jack's "looks ok to me "
> as Acked-by the pm8001 maintainer for libsas changes that don't touch
> drivers/scsi/pm8001.
Right, so reviewed by or approved by the maintainer == acked-by.
> For internal acks we should probably aim for promoting to Reviewed-by
> or Tested-by... if Acked-by is unwelcome in scsi.
We're just struggling to understand why it's there. If it's read and
approved the patch, then reviewed-by is the more appropriate. If it's
actually booted and ran through a set of unit/QA tests, then it should
be tested-by.
James
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 03/12] libata, libsas: introduce sched_eh and end_eh port ops
2012-04-23 22:22 ` James Bottomley
@ 2012-04-23 22:49 ` Dan Williams
2012-04-24 10:11 ` Jacek Danecki
0 siblings, 1 reply; 34+ messages in thread
From: Dan Williams @ 2012-04-23 22:49 UTC (permalink / raw)
To: James Bottomley
Cc: Jeff Garzik, Tejun Heo, linux-ide, linux-scsi, Jacek Danecki
On Mon, Apr 23, 2012 at 3:22 PM, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
>> No, it means:
>>
>> "If a person was not directly involved in the preparation or handling of a
>> patch but wishes to signify and record their approval of it then they can
>> arrange to have an Acked-by: line added to the patch's changelog."
>
> Isn't that tested-by or reviewed-by?
Quoting from Documentation/SubmittingPatches was just a tongue in
cheek way of pointing out that you have a local/narrower
interpretation of Acked-by, and that Jacek's Acked-by is consistent
with what's documented.
[..]
> We're just struggling to understand why it's there. If it's read and
> approved the patch, then reviewed-by is the more appropriate. If it's
> actually booted and ran through a set of unit/QA tests, then it should
> be tested-by.
>
Ok, reviewed-by is what we'll aim to do for Intel-internal "acks" for
isci / libsas going forward.
--
Dan
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 03/12] libata, libsas: introduce sched_eh and end_eh port ops
2012-04-22 17:30 ` James Bottomley
2012-04-23 2:33 ` Jeff Garzik
@ 2012-04-23 19:41 ` Dan Williams
2012-04-26 17:21 ` Dan Williams
1 sibling, 1 reply; 34+ messages in thread
From: Dan Williams @ 2012-04-23 19:41 UTC (permalink / raw)
To: James Bottomley; +Cc: Tejun Heo, linux-ide, linux-scsi, Jacek Danecki
On Sun, Apr 22, 2012 at 10:30 AM, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
> On Fri, 2012-04-13 at 16:37 -0700, Dan Williams wrote:
>> When managing shost->host_eh_scheduled libata assumes that there is a
>> 1:1 shost-to-ata_port relationship. libsas creates a 1:N relationship
>> so it needs to manage host_eh_scheduled cumulatively at the host level.
>> The sched_eh and end_eh port port ops allow libsas to track when domain
>> devices enter/leave the "eh-pending" state under ha->lock (previously
>> named ha->state_lock, but it is no longer just a lock for ha->state
>> changes).
>>
>> Since host_eh_scheduled indicates eh without backing commands pinning
>> the device it can be deallocated at any time. Move the taking of the
>> domain_device reference under the port_lock to guarantee that the
>> ata_port stays around for the duration of eh.
>
>> Cc: Tejun Heo <tj@kernel.org>
>> Acked-by: Jacek Danecki <jacek.danecki@intel.com>
>
> Could we standardise on Acked-by, please. In my book it means the
> maintainer of a piece of code agrees with the change and lets me take it
> through my tree. I'm aware that not everyone uses this definition, so
> we can use a different standard from my current one, but what does it
> mean in this case?
>
>> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>> ---
>> drivers/ata/libata-core.c | 4 ++
>> drivers/ata/libata-eh.c | 57 ++++++++++++++++++++++++++++-------
>> drivers/scsi/libsas/sas_ata.c | 38 +++++++++++++++++++++--
>> drivers/scsi/libsas/sas_discover.c | 6 ++--
>> drivers/scsi/libsas/sas_event.c | 12 ++++---
>> drivers/scsi/libsas/sas_init.c | 14 ++++-----
>> drivers/scsi/libsas/sas_scsi_host.c | 27 +++++++++++++----
>> include/linux/libata.h | 4 ++
>> include/scsi/libsas.h | 4 ++
>> include/scsi/sas_ata.h | 5 +++
>> 10 files changed, 134 insertions(+), 37 deletions(-)
>
> This is a pretty big change for rc fixes. None of the other changes in
> the series seem to be dependent on it, what bug is it actually fixing?
It fixes two bugs (which in hindsight could potentially be split into
two patches). The major one is guarantees about when
host_eh_scheduled is cleared. Prior to this patch when at least one
ata_port in a domain has completed eh the flag for host is cleared.
This patch plus "scsi: fix eh wakeup (scsi_schedule_eh vs
scsi_restart_operations)" fixes up deadlocks (waiting indefinitely for
eh to complete) and cases where eh terminates too early
(host_eh_scheduled cleared with work pending) in our hot plug tests.
The other bug this uncovered is the fact that libsas makes use of the
port and rphy object after libata has completed it's work, so this
patch also moved the taking of the domain_device reference to be under
spin_lock_irq(&sas_ha->phy_port_lock) and
spin_lock(&port->dev_list_lock). Otherwise, if no commands are
pending for the device libsas can proceed directly to freeing the
domain_device before the requested eh runs.
--
Dan
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 03/12] libata, libsas: introduce sched_eh and end_eh port ops
2012-04-23 19:41 ` Dan Williams
@ 2012-04-26 17:21 ` Dan Williams
0 siblings, 0 replies; 34+ messages in thread
From: Dan Williams @ 2012-04-26 17:21 UTC (permalink / raw)
To: James Bottomley; +Cc: Tejun Heo, linux-ide, linux-scsi, Jacek Danecki
On Mon, Apr 23, 2012 at 12:41 PM, Dan Williams <dan.j.williams@intel.com> wrote:
> On Sun, Apr 22, 2012 at 10:30 AM, James Bottomley
> <James.Bottomley@hansenpartnership.com> wrote:
>> On Fri, 2012-04-13 at 16:37 -0700, Dan Williams wrote:
>>> When managing shost->host_eh_scheduled libata assumes that there is a
>>> 1:1 shost-to-ata_port relationship. libsas creates a 1:N relationship
>>> so it needs to manage host_eh_scheduled cumulatively at the host level.
>>> The sched_eh and end_eh port port ops allow libsas to track when domain
>>> devices enter/leave the "eh-pending" state under ha->lock (previously
>>> named ha->state_lock, but it is no longer just a lock for ha->state
>>> changes).
>>>
>>> Since host_eh_scheduled indicates eh without backing commands pinning
>>> the device it can be deallocated at any time. Move the taking of the
>>> domain_device reference under the port_lock to guarantee that the
>>> ata_port stays around for the duration of eh.
>>
>>> Cc: Tejun Heo <tj@kernel.org>
>>> Acked-by: Jacek Danecki <jacek.danecki@intel.com>
>>
>> Could we standardise on Acked-by, please. In my book it means the
>> maintainer of a piece of code agrees with the change and lets me take it
>> through my tree. I'm aware that not everyone uses this definition, so
>> we can use a different standard from my current one, but what does it
>> mean in this case?
>>
>>> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>>> ---
>>> drivers/ata/libata-core.c | 4 ++
>>> drivers/ata/libata-eh.c | 57 ++++++++++++++++++++++++++++-------
>>> drivers/scsi/libsas/sas_ata.c | 38 +++++++++++++++++++++--
>>> drivers/scsi/libsas/sas_discover.c | 6 ++--
>>> drivers/scsi/libsas/sas_event.c | 12 ++++---
>>> drivers/scsi/libsas/sas_init.c | 14 ++++-----
>>> drivers/scsi/libsas/sas_scsi_host.c | 27 +++++++++++++----
>>> include/linux/libata.h | 4 ++
>>> include/scsi/libsas.h | 4 ++
>>> include/scsi/sas_ata.h | 5 +++
>>> 10 files changed, 134 insertions(+), 37 deletions(-)
>>
>> This is a pretty big change for rc fixes. None of the other changes in
>> the series seem to be dependent on it, what bug is it actually fixing?
>
> It fixes two bugs (which in hindsight could potentially be split into
> two patches). The major one is guarantees about when
> host_eh_scheduled is cleared. Prior to this patch when at least one
> ata_port in a domain has completed eh the flag for host is cleared.
> This patch plus "scsi: fix eh wakeup (scsi_schedule_eh vs
> scsi_restart_operations)" fixes up deadlocks (waiting indefinitely for
> eh to complete) and cases where eh terminates too early
> (host_eh_scheduled cleared with work pending) in our hot plug tests.
>
> The other bug this uncovered is the fact that libsas makes use of the
> port and rphy object after libata has completed it's work, so this
> patch also moved the taking of the domain_device reference to be under
> spin_lock_irq(&sas_ha->phy_port_lock) and
> spin_lock(&port->dev_list_lock). Otherwise, if no commands are
> pending for the device libsas can proceed directly to freeing the
> domain_device before the requested eh runs.
>
Ping, will this one be queued to scsi/fixes? All our hotplug testing
was done with this patch in place, and it's straightforward to see
that libata is mismanaging host_eh_scheduled in the libsas ata_port
case.
--
Dan
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 04/12] libsas: fix sas_find_bcast_phy() in the presence of 'vacant' phys
2012-04-13 23:36 [GIT PATCH 00/12] libsas fixes for 3.4 Dan Williams
` (2 preceding siblings ...)
2012-04-13 23:37 ` [PATCH 03/12] libata, libsas: introduce sched_eh and end_eh port ops Dan Williams
@ 2012-04-13 23:37 ` Dan Williams
2012-04-13 23:37 ` [PATCH 05/12] libsas: fix sas_get_port_device regression Dan Williams
` (8 subsequent siblings)
12 siblings, 0 replies; 34+ messages in thread
From: Dan Williams @ 2012-04-13 23:37 UTC (permalink / raw)
To: JBottomley; +Cc: Thomas Jackson, linux-ide, stable, linux-scsi
From: Thomas Jackson <thomas.p.jackson@intel.com>
If an expander reports 'PHY VACANT' for a phy index prior to the one
that generated a BCN libsas fails rediscovery. Since a vacant phy is
defined as a valid phy index that will never have an attached device
just continue the search.
Cc: <stable@vger.kernel.org>
Signed-off-by: Thomas Jackson <thomas.p.jackson@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/scsi/libsas/sas_expander.c | 17 ++++++++++++-----
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index 05acd9e..833bea0 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -1718,9 +1718,17 @@ static int sas_find_bcast_phy(struct domain_device *dev, int *phy_id,
int phy_change_count = 0;
res = sas_get_phy_change_count(dev, i, &phy_change_count);
- if (res)
- goto out;
- else if (phy_change_count != ex->ex_phy[i].phy_change_count) {
+ switch (res) {
+ case SMP_RESP_PHY_VACANT:
+ case SMP_RESP_NO_PHY:
+ continue;
+ case SMP_RESP_FUNC_ACC:
+ break;
+ default:
+ return res;
+ }
+
+ if (phy_change_count != ex->ex_phy[i].phy_change_count) {
if (update)
ex->ex_phy[i].phy_change_count =
phy_change_count;
@@ -1728,8 +1736,7 @@ static int sas_find_bcast_phy(struct domain_device *dev, int *phy_id,
return 0;
}
}
-out:
- return res;
+ return 0;
}
static int sas_get_ex_change_count(struct domain_device *dev, int *ecc)
^ permalink raw reply related [flat|nested] 34+ messages in thread* [PATCH 05/12] libsas: fix sas_get_port_device regression
2012-04-13 23:36 [GIT PATCH 00/12] libsas fixes for 3.4 Dan Williams
` (3 preceding siblings ...)
2012-04-13 23:37 ` [PATCH 04/12] libsas: fix sas_find_bcast_phy() in the presence of 'vacant' phys Dan Williams
@ 2012-04-13 23:37 ` Dan Williams
2012-04-13 23:37 ` [PATCH 06/12] libsas: unify domain_device sas_rphy lifetimes Dan Williams
` (7 subsequent siblings)
12 siblings, 0 replies; 34+ messages in thread
From: Dan Williams @ 2012-04-13 23:37 UTC (permalink / raw)
To: JBottomley; +Cc: Tom Jackson, linux-ide, linux-scsi
Commit 899fcf4 "[SCSI] libsas: set attached device type and target
protocols for local phys" setup 'phy' to be dereferenced after
list_for_each_entry(phy, &port->phy_list, port_phy_el) (i.e. phy ==
&port->phy_list) resulting in reports like:
BUG: unable to handle kernel NULL pointer dereference at 00000000000002b0
IP: [<ffffffffa00ce948>] sas_discover_domain+0x29e/0x4fb [libsas]
...fix by deferring sas_phy_set_target() to the end of
sas_get_port_device().
Reported-by: Tom Jackson <thomas.p.jackson@intel.com>
Tested-by: Tom Jackson <thomas.p.jackson@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/scsi/libsas/sas_discover.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/scsi/libsas/sas_discover.c b/drivers/scsi/libsas/sas_discover.c
index b82949c..4da5ea8 100644
--- a/drivers/scsi/libsas/sas_discover.c
+++ b/drivers/scsi/libsas/sas_discover.c
@@ -134,10 +134,6 @@ static int sas_get_port_device(struct asd_sas_port *port)
return -ENODEV;
}
- spin_lock_irq(&port->phy_list_lock);
- list_for_each_entry(phy, &port->phy_list, port_phy_el)
- sas_phy_set_target(phy, dev);
- spin_unlock_irq(&port->phy_list_lock);
rphy->identify.phy_identifier = phy->phy->identify.phy_identifier;
memcpy(dev->sas_addr, port->attached_sas_addr, SAS_ADDR_SIZE);
sas_fill_in_rphy(dev, rphy);
@@ -164,6 +160,11 @@ static int sas_get_port_device(struct asd_sas_port *port)
spin_unlock_irq(&port->dev_list_lock);
}
+ spin_lock_irq(&port->phy_list_lock);
+ list_for_each_entry(phy, &port->phy_list, port_phy_el)
+ sas_phy_set_target(phy, dev);
+ spin_unlock_irq(&port->phy_list_lock);
+
return 0;
}
^ permalink raw reply related [flat|nested] 34+ messages in thread* [PATCH 06/12] libsas: unify domain_device sas_rphy lifetimes
2012-04-13 23:36 [GIT PATCH 00/12] libsas fixes for 3.4 Dan Williams
` (4 preceding siblings ...)
2012-04-13 23:37 ` [PATCH 05/12] libsas: fix sas_get_port_device regression Dan Williams
@ 2012-04-13 23:37 ` Dan Williams
2012-04-13 23:37 ` [PATCH 07/12] libsas: fix ata_eh clobbering ex_phys via smp_ata_check_ready Dan Williams
` (6 subsequent siblings)
12 siblings, 0 replies; 34+ messages in thread
From: Dan Williams @ 2012-04-13 23:37 UTC (permalink / raw)
To: JBottomley; +Cc: Tom Jackson, linux-ide, linux-scsi
Since the domain_device can out live the scsi_target we need the rphy to
follow suit otherwise we run into issues like:
BUG: unable to handle kernel NULL pointer dereference at 0000000000000050
IP: [<ffffffffa011561b>] sas_ata_printk+0x43/0x6f [libsas]
PGD 0
Oops: 0000 [#1] SMP
CPU 1
Modules linked in: ses enclosure isci libsas scsi_transport_sas fuse sunrpc cpufreq_ondemand acpi_cpufreq freq_table mperf microcode pcspkr igb joydev iTCO_wdt ioatdma iTCO_vendor_support i2c_i801 i2c_core dca wmi hed ipv6 pata_acpi ata_generic [last unloaded: scsi_wait_scan]
Pid: 129, comm: kworker/u:3 Not tainted 3.3.0-rc5-isci+ #1 Intel Corporation SandyBridge Platform/To be filled by O.E.M.
RIP: 0010:[<ffffffffa011561b>] [<ffffffffa011561b>] sas_ata_printk+0x43/0x6f [libsas]
RSP: 0018:ffff88042232dd70 EFLAGS: 00010282
RAX: 0000000000000000 RBX: ffff8804283165b8 RCX: ffff88042232dda0
RDX: ffff88042232dd78 RSI: ffff8804283165b8 RDI: ffffffffa01188d7
RBP: ffff88042232ddd0 R08: ffff880388454000 R09: ffff8803edfde1f8
R10: ffff8803edfde1f8 R11: ffff8803edfde1f8 R12: ffff880428316750
R13: ffff880388454000 R14: ffff8803f88b31d0 R15: ffff8803f8b21d50
FS: 0000000000000000(0000) GS:ffff88042ee20000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 0000000000000050 CR3: 0000000001a05000 CR4: 00000000000406e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process kworker/u:3 (pid: 129, threadinfo ffff88042232c000, task ffff88042230c920)
Stack:
0000000000000000 ffff880400000018 ffff88042232dde0 ffff88042232dda0
ffffffffa01188c4 ffff88042ee93af0 ffff88042232ddb0 ffffffff8100e047
ffff88042232de10 ffff880420e5a2c8 ffff8803f8b21d50 ffff8803edfde1f8
Call Trace:
[<ffffffff8100e047>] ? load_TLS+0xb/0xf
[<ffffffffa01156ad>] async_sas_ata_eh+0x66/0x95 [libsas]
[<ffffffff810655e1>] async_run_entry_fn+0x9e/0x131
Reported-by: Tom Jackson <thomas.p.jackson@intel.com>
Tested-by: Tom Jackson <thomas.p.jackson@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/scsi/libsas/sas_discover.c | 11 ++++++-----
drivers/scsi/libsas/sas_expander.c | 6 ++++--
2 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/drivers/scsi/libsas/sas_discover.c b/drivers/scsi/libsas/sas_discover.c
index 4da5ea8..128160d 100644
--- a/drivers/scsi/libsas/sas_discover.c
+++ b/drivers/scsi/libsas/sas_discover.c
@@ -151,6 +151,7 @@ static int sas_get_port_device(struct asd_sas_port *port)
sas_device_set_phy(dev, port->port);
dev->rphy = rphy;
+ get_device(&dev->rphy->dev);
if (dev_is_sata(dev) || dev->dev_type == SAS_END_DEV)
list_add_tail(&dev->disco_list_node, &port->disco_list);
@@ -255,6 +256,9 @@ void sas_free_device(struct kref *kref)
{
struct domain_device *dev = container_of(kref, typeof(*dev), kref);
+ put_device(&dev->rphy->dev);
+ dev->rphy = NULL;
+
if (dev->parent)
sas_put_device(dev->parent);
@@ -303,7 +307,6 @@ static void sas_destruct_devices(struct work_struct *work)
sas_remove_children(&dev->rphy->dev);
sas_rphy_delete(dev->rphy);
- dev->rphy = NULL;
sas_unregister_common_dev(port, dev);
}
}
@@ -315,11 +318,11 @@ void sas_unregister_dev(struct asd_sas_port *port, struct domain_device *dev)
/* this rphy never saw sas_rphy_add */
list_del_init(&dev->disco_list_node);
sas_rphy_free(dev->rphy);
- dev->rphy = NULL;
sas_unregister_common_dev(port, dev);
+ return;
}
- if (dev->rphy && !test_and_set_bit(SAS_DEV_DESTROY, &dev->state)) {
+ 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);
@@ -419,8 +422,6 @@ static void sas_discover_domain(struct work_struct *work)
if (error) {
sas_rphy_free(dev->rphy);
- dev->rphy = NULL;
-
list_del_init(&dev->disco_list_node);
spin_lock_irq(&port->dev_list_lock);
list_del_init(&dev->dev_list_node);
diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index 833bea0..37c3c3f 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -783,6 +783,7 @@ static struct domain_device *sas_ex_discover_end_dev(
sas_init_dev(child);
child->rphy = rphy;
+ get_device(&rphy->dev);
list_add_tail(&child->disco_list_node, &parent->port->disco_list);
@@ -806,6 +807,7 @@ static struct domain_device *sas_ex_discover_end_dev(
sas_init_dev(child);
child->rphy = rphy;
+ get_device(&rphy->dev);
sas_fill_in_rphy(child, rphy);
list_add_tail(&child->disco_list_node, &parent->port->disco_list);
@@ -830,8 +832,6 @@ static struct domain_device *sas_ex_discover_end_dev(
out_list_del:
sas_rphy_free(child->rphy);
- child->rphy = NULL;
-
list_del(&child->disco_list_node);
spin_lock_irq(&parent->port->dev_list_lock);
list_del(&child->dev_list_node);
@@ -911,6 +911,7 @@ static struct domain_device *sas_ex_discover_expander(
}
port = parent->port;
child->rphy = rphy;
+ get_device(&rphy->dev);
edev = rphy_to_expander_device(rphy);
child->dev_type = phy->attached_dev_type;
kref_get(&parent->kref);
@@ -934,6 +935,7 @@ static struct domain_device *sas_ex_discover_expander(
res = sas_discover_expander(child);
if (res) {
+ sas_rphy_delete(rphy);
spin_lock_irq(&parent->port->dev_list_lock);
list_del(&child->dev_list_node);
spin_unlock_irq(&parent->port->dev_list_lock);
^ permalink raw reply related [flat|nested] 34+ messages in thread* [PATCH 07/12] libsas: fix ata_eh clobbering ex_phys via smp_ata_check_ready
2012-04-13 23:36 [GIT PATCH 00/12] libsas fixes for 3.4 Dan Williams
` (5 preceding siblings ...)
2012-04-13 23:37 ` [PATCH 06/12] libsas: unify domain_device sas_rphy lifetimes Dan Williams
@ 2012-04-13 23:37 ` Dan Williams
2012-04-13 23:37 ` [PATCH 08/12] libata: make ata_print_id atomic Dan Williams
` (5 subsequent siblings)
12 siblings, 0 replies; 34+ messages in thread
From: Dan Williams @ 2012-04-13 23:37 UTC (permalink / raw)
To: JBottomley
Cc: Maciej Patelczyk, linux-scsi, Jacek Danecki, Jack Wang,
linux-ide, Bartek Nowakowski
The check_ready implementation in the expander-attached ata device case
polls on sas_ex_phy_discover(). The effect is that the ex_phy fields
(critically ->attached_sas_addr) can change. When ata_eh ends and
libsas comes along to revalidate the domain
sas_unregister_devs_sas_addr() can fail to lookup devices to remove, or
fail to re-add an ata device that ata_eh marked as disabled. So change
the code to skip the sas_address and change count updates when ata_eh is
active.
Cc: Jack Wang <jack_wang@usish.com>
Tested-by: Maciej Patelczyk <maciej.patelczyk@intel.com>
Tested-by: Bartek Nowakowski <bartek.nowakowski@intel.com>
Tested-by: Jacek Danecki <jacek.danecki@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/scsi/libsas/sas_expander.c | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index 37c3c3f..c1f91b1 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -202,6 +202,7 @@ static void sas_set_ex_phy(struct domain_device *dev, int phy_id, void *rsp)
u8 sas_addr[SAS_ADDR_SIZE];
struct smp_resp *resp = rsp;
struct discover_resp *dr = &resp->disc;
+ struct sas_ha_struct *ha = dev->port->ha;
struct expander_device *ex = &dev->ex_dev;
struct ex_phy *phy = &ex->ex_phy[phy_id];
struct sas_rphy *rphy = dev->rphy;
@@ -209,6 +210,8 @@ static void sas_set_ex_phy(struct domain_device *dev, int phy_id, void *rsp)
char *type;
if (new_phy) {
+ if (WARN_ON_ONCE(test_bit(SAS_HA_ATA_EH_ACTIVE, &ha->state)))
+ return;
phy->phy = sas_phy_alloc(&rphy->dev, phy_id);
/* FIXME: error_handling */
@@ -233,6 +236,8 @@ static void sas_set_ex_phy(struct domain_device *dev, int phy_id, void *rsp)
memcpy(sas_addr, phy->attached_sas_addr, SAS_ADDR_SIZE);
phy->attached_dev_type = to_dev_type(dr);
+ if (test_bit(SAS_HA_ATA_EH_ACTIVE, &ha->state))
+ goto out;
phy->phy_id = phy_id;
phy->linkrate = dr->linkrate;
phy->attached_sata_host = dr->attached_sata_host;
@@ -266,6 +271,7 @@ static void sas_set_ex_phy(struct domain_device *dev, int phy_id, void *rsp)
return;
}
+ out:
switch (phy->attached_dev_type) {
case SATA_PENDING:
type = "stp pending";
@@ -304,7 +310,15 @@ static void sas_set_ex_phy(struct domain_device *dev, int phy_id, void *rsp)
else
return;
- SAS_DPRINTK("ex %016llx phy%02d:%c:%X attached: %016llx (%s)\n",
+ /* if the attached device type changed and ata_eh is active,
+ * make sure we run revalidation when eh completes (see:
+ * sas_enable_revalidation)
+ */
+ if (test_bit(SAS_HA_ATA_EH_ACTIVE, &ha->state))
+ set_bit(DISCE_REVALIDATE_DOMAIN, &dev->port->disc.pending);
+
+ SAS_DPRINTK("%sex %016llx phy%02d:%c:%X attached: %016llx (%s)\n",
+ test_bit(SAS_HA_ATA_EH_ACTIVE, &ha->state) ? "ata: " : "",
SAS_ADDR(dev->sas_addr), phy->phy_id,
sas_route_char(dev, phy), phy->linkrate,
SAS_ADDR(phy->attached_sas_addr), type);
^ permalink raw reply related [flat|nested] 34+ messages in thread* [PATCH 08/12] libata: make ata_print_id atomic
2012-04-13 23:36 [GIT PATCH 00/12] libsas fixes for 3.4 Dan Williams
` (6 preceding siblings ...)
2012-04-13 23:37 ` [PATCH 07/12] libsas: fix ata_eh clobbering ex_phys via smp_ata_check_ready Dan Williams
@ 2012-04-13 23:37 ` Dan Williams
2012-04-13 23:37 ` [PATCH 09/12] libsas, libata: fix start of life for a sas ata_port Dan Williams
` (4 subsequent siblings)
12 siblings, 0 replies; 34+ messages in thread
From: Dan Williams @ 2012-04-13 23:37 UTC (permalink / raw)
To: JBottomley; +Cc: linux-ide, linux-scsi, Jacek Danecki
This variable is incremented from multiple contexts (module_init via
libata-lldds and the libsas discovery thread). Make it atomic to head
off any chance of libsas and libata creating duplicate ids.
Acked-by: Jacek Danecki <jacek.danecki@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/ata/libata-core.c | 4 ++--
drivers/ata/libata-scsi.c | 4 ++--
drivers/ata/libata.h | 2 +-
3 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index ea8444e..bd79c8b 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -97,7 +97,7 @@ static unsigned int ata_dev_set_xfermode(struct ata_device *dev);
static void ata_dev_xfermask(struct ata_device *dev);
static unsigned long ata_dev_blacklisted(const struct ata_device *dev);
-unsigned int ata_print_id = 1;
+atomic_t ata_print_id = ATOMIC_INIT(1);
struct ata_force_param {
const char *name;
@@ -6031,7 +6031,7 @@ int ata_host_register(struct ata_host *host, struct scsi_host_template *sht)
/* give ports names and add SCSI hosts */
for (i = 0; i < host->n_ports; i++)
- host->ports[i]->print_id = ata_print_id++;
+ host->ports[i]->print_id = atomic_inc_return(&ata_print_id);
/* Create associated sysfs transport objects */
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 1ee00c8..93dabdc 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3843,7 +3843,7 @@ int ata_sas_async_port_init(struct ata_port *ap)
int rc = ap->ops->port_start(ap);
if (!rc) {
- ap->print_id = ata_print_id++;
+ ap->print_id = atomic_inc_return(&ata_print_id);
__ata_port_probe(ap);
}
@@ -3867,7 +3867,7 @@ int ata_sas_port_init(struct ata_port *ap)
int rc = ap->ops->port_start(ap);
if (!rc) {
- ap->print_id = ata_print_id++;
+ ap->print_id = atomic_inc_return(&ata_print_id);
rc = ata_port_probe(ap);
}
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index 2e26fca..9d0fd0b 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -53,7 +53,7 @@ enum {
ATA_DNXFER_QUIET = (1 << 31),
};
-extern unsigned int ata_print_id;
+extern atomic_t ata_print_id;
extern int atapi_passthru16;
extern int libata_fua;
extern int libata_noacpi;
^ permalink raw reply related [flat|nested] 34+ messages in thread* [PATCH 09/12] libsas, libata: fix start of life for a sas ata_port
2012-04-13 23:36 [GIT PATCH 00/12] libsas fixes for 3.4 Dan Williams
` (7 preceding siblings ...)
2012-04-13 23:37 ` [PATCH 08/12] libata: make ata_print_id atomic Dan Williams
@ 2012-04-13 23:37 ` Dan Williams
2012-04-21 6:20 ` Jeff Garzik
2012-04-13 23:37 ` [PATCH 10/12] scsi: fix eh wakeup (scsi_schedule_eh vs scsi_restart_operations) Dan Williams
` (3 subsequent siblings)
12 siblings, 1 reply; 34+ messages in thread
From: Dan Williams @ 2012-04-13 23:37 UTC (permalink / raw)
To: JBottomley; +Cc: linux-ide, Michal Kosciowski, linux-scsi
This changes the ordering of initialization and probing events from:
1/ allocate rphy in PORTE_BYTES_DMAED, DISCE_REVALIDATE_DOMAIN
2/ allocate ata_port and schedule port probe in DISCE_PROBE
...to:
1/ allocate ata_port in PORTE_BYTES_DMAED, DISCE_REVALIDATE_DOMAIN
2/ allocate rphy in PORTE_BYTES_DMAED, DISCE_REVALIDATE_DOMAIN
3/ schedule port probe in DISCE_PROBE
This ordering prevents PHYE_SIGNAL_LOSS_EVENTS from sneaking in to
destrory ata devices before they have been fully initialized:
BUG: unable to handle kernel paging request at 0000000000003b10
IP: [<ffffffffa0053d7e>] sas_ata_end_eh+0x12/0x5e [libsas]
...
[<ffffffffa004d1af>] sas_unregister_common_dev+0x78/0xc9 [libsas]
[<ffffffffa004d4d4>] sas_unregister_dev+0x4f/0xad [libsas]
[<ffffffffa004d5b1>] sas_unregister_domain_devices+0x7f/0xbf [libsas]
[<ffffffffa004c487>] sas_deform_port+0x61/0x1b8 [libsas]
[<ffffffffa004bed0>] sas_phye_loss_of_signal+0x29/0x2b [libsas]
...and kills the awkward "sata domain_device briefly existing in the
domain without an ata_port" state.
Reported-by: Michal Kosciowski <michal.kosciowski@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/ata/libata-scsi.c | 35 ++++++++++++++++++++---------------
drivers/scsi/ipr.c | 6 +++++-
drivers/scsi/libsas/sas_ata.c | 33 ++++++++++-----------------------
drivers/scsi/libsas/sas_discover.c | 13 ++++++++++---
drivers/scsi/libsas/sas_expander.c | 8 +++++---
include/linux/libata.h | 3 ++-
include/scsi/sas_ata.h | 4 ++--
7 files changed, 54 insertions(+), 48 deletions(-)
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 93dabdc..cde63f2 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3838,18 +3838,25 @@ void ata_sas_port_stop(struct ata_port *ap)
}
EXPORT_SYMBOL_GPL(ata_sas_port_stop);
-int ata_sas_async_port_init(struct ata_port *ap)
+/**
+ * ata_sas_async_probe - simply schedule probing and return
+ * @ap: Port to probe
+ *
+ * For batch scheduling of probe for sas attached ata devices, assumes
+ * the port has already been through ata_sas_port_init()
+ */
+void ata_sas_async_probe(struct ata_port *ap)
{
- int rc = ap->ops->port_start(ap);
-
- if (!rc) {
- ap->print_id = atomic_inc_return(&ata_print_id);
- __ata_port_probe(ap);
- }
+ __ata_port_probe(ap);
+}
+EXPORT_SYMBOL_GPL(ata_sas_async_probe);
- return rc;
+int ata_sas_sync_probe(struct ata_port *ap)
+{
+ return ata_port_probe(ap);
}
-EXPORT_SYMBOL_GPL(ata_sas_async_port_init);
+EXPORT_SYMBOL_GPL(ata_sas_sync_probe);
+
/**
* ata_sas_port_init - Initialize a SATA device
@@ -3866,12 +3873,10 @@ int ata_sas_port_init(struct ata_port *ap)
{
int rc = ap->ops->port_start(ap);
- if (!rc) {
- ap->print_id = atomic_inc_return(&ata_print_id);
- rc = ata_port_probe(ap);
- }
-
- return rc;
+ if (rc)
+ return rc;
+ ap->print_id = atomic_inc_return(&ata_print_id);
+ return 0;
}
EXPORT_SYMBOL_GPL(ata_sas_port_init);
diff --git a/drivers/scsi/ipr.c b/drivers/scsi/ipr.c
index cdfe5a1..4c49769 100644
--- a/drivers/scsi/ipr.c
+++ b/drivers/scsi/ipr.c
@@ -4546,8 +4546,12 @@ static int ipr_ata_slave_alloc(struct scsi_device *sdev)
ENTER;
if (sdev->sdev_target)
sata_port = sdev->sdev_target->hostdata;
- if (sata_port)
+ if (sata_port) {
rc = ata_sas_port_init(sata_port->ap);
+ if (rc == 0)
+ rc = ata_sas_sync_probe(sata_port->ap);
+ }
+
if (rc)
ipr_slave_destroy(sdev);
diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index 545b78d..6a6c80f 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -573,11 +573,12 @@ static struct ata_port_info sata_port_info = {
.port_ops = &sas_sata_ops
};
-int sas_ata_init_host_and_port(struct domain_device *found_dev)
+int sas_ata_init(struct domain_device *found_dev)
{
struct sas_ha_struct *ha = found_dev->port->ha;
struct Scsi_Host *shost = ha->core.shost;
struct ata_port *ap;
+ int rc;
ata_host_init(&found_dev->sata_dev.ata_host,
ha->dev,
@@ -594,8 +595,11 @@ int sas_ata_init_host_and_port(struct domain_device *found_dev)
ap->private_data = found_dev;
ap->cbl = ATA_CBL_SATA;
ap->scsi_host = shost;
- /* publish initialized ata port */
- smp_wmb();
+ rc = ata_sas_port_init(ap);
+ if (rc) {
+ ata_sas_port_destroy(ap);
+ return rc;
+ }
found_dev->sata_dev.ap = ap;
return 0;
@@ -674,18 +678,13 @@ static void sas_get_ata_command_set(struct domain_device *dev)
void sas_probe_sata(struct asd_sas_port *port)
{
struct domain_device *dev, *n;
- int err;
mutex_lock(&port->ha->disco_mutex);
- list_for_each_entry_safe(dev, n, &port->disco_list, disco_list_node) {
+ list_for_each_entry(dev, &port->disco_list, disco_list_node) {
if (!dev_is_sata(dev))
continue;
- err = sas_ata_init_host_and_port(dev);
- if (err)
- sas_fail_probe(dev, __func__, err);
- else
- ata_sas_async_port_init(dev->sata_dev.ap);
+ ata_sas_async_probe(dev->sata_dev.ap);
}
mutex_unlock(&port->ha->disco_mutex);
@@ -740,18 +739,6 @@ static void async_sas_ata_eh(void *data, async_cookie_t cookie)
sas_put_device(dev);
}
-static bool sas_ata_dev_eh_valid(struct domain_device *dev)
-{
- struct ata_port *ap;
-
- if (!dev_is_sata(dev))
- return false;
- ap = dev->sata_dev.ap;
- /* consume fully initialized ata ports */
- smp_rmb();
- return !!ap;
-}
-
void sas_ata_strategy_handler(struct Scsi_Host *shost)
{
struct sas_ha_struct *sas_ha = SHOST_TO_SAS_HA(shost);
@@ -775,7 +762,7 @@ void sas_ata_strategy_handler(struct Scsi_Host *shost)
spin_lock(&port->dev_list_lock);
list_for_each_entry(dev, &port->dev_list, dev_list_node) {
- if (!sas_ata_dev_eh_valid(dev))
+ if (!dev_is_sata(dev))
continue;
/* hold a reference over eh since we may be
diff --git a/drivers/scsi/libsas/sas_discover.c b/drivers/scsi/libsas/sas_discover.c
index 128160d..ff497ac 100644
--- a/drivers/scsi/libsas/sas_discover.c
+++ b/drivers/scsi/libsas/sas_discover.c
@@ -72,6 +72,7 @@ static int sas_get_port_device(struct asd_sas_port *port)
struct asd_sas_phy *phy;
struct sas_rphy *rphy;
struct domain_device *dev;
+ int rc = -ENODEV;
dev = sas_alloc_device();
if (!dev)
@@ -110,9 +111,16 @@ static int sas_get_port_device(struct asd_sas_port *port)
sas_init_dev(dev);
+ dev->port = port;
switch (dev->dev_type) {
- case SAS_END_DEV:
case SATA_DEV:
+ rc = sas_ata_init(dev);
+ if (rc) {
+ rphy = NULL;
+ break;
+ }
+ /* fall through */
+ case SAS_END_DEV:
rphy = sas_end_device_alloc(port->port);
break;
case EDGE_DEV:
@@ -131,7 +139,7 @@ static int sas_get_port_device(struct asd_sas_port *port)
if (!rphy) {
sas_put_device(dev);
- return -ENODEV;
+ return rc;
}
rphy->identify.phy_identifier = phy->phy->identify.phy_identifier;
@@ -139,7 +147,6 @@ static int sas_get_port_device(struct asd_sas_port *port)
sas_fill_in_rphy(dev, rphy);
sas_hash_addr(dev->hashed_sas_addr, dev->sas_addr);
port->port_dev = dev;
- dev->port = port;
dev->linkrate = port->linkrate;
dev->min_linkrate = port->linkrate;
dev->max_linkrate = port->linkrate;
diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index c1f91b1..75247a1 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -790,12 +790,14 @@ static struct domain_device *sas_ex_discover_end_dev(
if (res)
goto out_free;
+ sas_init_dev(child);
+ res = sas_ata_init(child);
+ if (res)
+ goto out_free;
rphy = sas_end_device_alloc(phy->port);
- if (unlikely(!rphy))
+ if (!rphy)
goto out_free;
- sas_init_dev(child);
-
child->rphy = rphy;
get_device(&rphy->dev);
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 83ff256..a868bbc 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -998,7 +998,8 @@ extern int ata_sas_scsi_ioctl(struct ata_port *ap, struct scsi_device *dev,
extern void ata_sas_port_destroy(struct ata_port *);
extern struct ata_port *ata_sas_port_alloc(struct ata_host *,
struct ata_port_info *, struct Scsi_Host *);
-extern int ata_sas_async_port_init(struct ata_port *);
+extern void ata_sas_async_probe(struct ata_port *ap);
+extern int ata_sas_sync_probe(struct ata_port *ap);
extern int ata_sas_port_init(struct ata_port *);
extern int ata_sas_port_start(struct ata_port *ap);
extern void ata_sas_port_stop(struct ata_port *ap);
diff --git a/include/scsi/sas_ata.h b/include/scsi/sas_ata.h
index cae55b6..2dfbdaa 100644
--- a/include/scsi/sas_ata.h
+++ b/include/scsi/sas_ata.h
@@ -37,7 +37,7 @@ static inline int dev_is_sata(struct domain_device *dev)
}
int sas_get_ata_info(struct domain_device *dev, struct ex_phy *phy);
-int sas_ata_init_host_and_port(struct domain_device *found_dev);
+int sas_ata_init(struct domain_device *dev);
void sas_ata_task_abort(struct sas_task *task);
void sas_ata_strategy_handler(struct Scsi_Host *shost);
void sas_ata_eh(struct Scsi_Host *shost, struct list_head *work_q,
@@ -53,7 +53,7 @@ static inline int dev_is_sata(struct domain_device *dev)
{
return 0;
}
-static inline int sas_ata_init_host_and_port(struct domain_device *found_dev)
+static inline int sas_ata_init(struct domain_device *dev)
{
return 0;
}
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH 09/12] libsas, libata: fix start of life for a sas ata_port
2012-04-13 23:37 ` [PATCH 09/12] libsas, libata: fix start of life for a sas ata_port Dan Williams
@ 2012-04-21 6:20 ` Jeff Garzik
0 siblings, 0 replies; 34+ messages in thread
From: Jeff Garzik @ 2012-04-21 6:20 UTC (permalink / raw)
To: Dan Williams; +Cc: JBottomley, linux-ide, Michal Kosciowski, linux-scsi
On 04/13/2012 07:37 PM, Dan Williams wrote:
> This changes the ordering of initialization and probing events from:
> 1/ allocate rphy in PORTE_BYTES_DMAED, DISCE_REVALIDATE_DOMAIN
> 2/ allocate ata_port and schedule port probe in DISCE_PROBE
> ...to:
> 1/ allocate ata_port in PORTE_BYTES_DMAED, DISCE_REVALIDATE_DOMAIN
> 2/ allocate rphy in PORTE_BYTES_DMAED, DISCE_REVALIDATE_DOMAIN
> 3/ schedule port probe in DISCE_PROBE
>
> This ordering prevents PHYE_SIGNAL_LOSS_EVENTS from sneaking in to
> destrory ata devices before they have been fully initialized:
>
> BUG: unable to handle kernel paging request at 0000000000003b10
> IP: [<ffffffffa0053d7e>] sas_ata_end_eh+0x12/0x5e [libsas]
> ...
> [<ffffffffa004d1af>] sas_unregister_common_dev+0x78/0xc9 [libsas]
> [<ffffffffa004d4d4>] sas_unregister_dev+0x4f/0xad [libsas]
> [<ffffffffa004d5b1>] sas_unregister_domain_devices+0x7f/0xbf [libsas]
> [<ffffffffa004c487>] sas_deform_port+0x61/0x1b8 [libsas]
> [<ffffffffa004bed0>] sas_phye_loss_of_signal+0x29/0x2b [libsas]
>
> ...and kills the awkward "sata domain_device briefly existing in the
> domain without an ata_port" state.
>
> Reported-by: Michal Kosciowski<michal.kosciowski@intel.com>
> Signed-off-by: Dan Williams<dan.j.williams@intel.com>
> ---
> drivers/ata/libata-scsi.c | 35 ++++++++++++++++++++---------------
> drivers/scsi/ipr.c | 6 +++++-
> drivers/scsi/libsas/sas_ata.c | 33 ++++++++++-----------------------
> drivers/scsi/libsas/sas_discover.c | 13 ++++++++++---
> drivers/scsi/libsas/sas_expander.c | 8 +++++---
> include/linux/libata.h | 3 ++-
> include/scsi/sas_ata.h | 4 ++--
> 7 files changed, 54 insertions(+), 48 deletions(-)
Acked-by: Jeff Garzik <jgarzik@redhat.com>
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 10/12] scsi: fix eh wakeup (scsi_schedule_eh vs scsi_restart_operations)
2012-04-13 23:36 [GIT PATCH 00/12] libsas fixes for 3.4 Dan Williams
` (8 preceding siblings ...)
2012-04-13 23:37 ` [PATCH 09/12] libsas, libata: fix start of life for a sas ata_port Dan Williams
@ 2012-04-13 23:37 ` Dan Williams
2012-04-21 12:22 ` James Bottomley
2012-04-13 23:37 ` [PATCH 11/12] libsas: fix false positive 'device attached' conditions Dan Williams
` (2 subsequent siblings)
12 siblings, 1 reply; 34+ messages in thread
From: Dan Williams @ 2012-04-13 23:37 UTC (permalink / raw)
To: JBottomley; +Cc: Tejun Heo, linux-ide, Tom Jackson, linux-scsi
Rapid ata hotplug on a libsas controller results in cases where libsas
is waiting indefinitely on eh to perform an ata probe.
A race exists between scsi_schedule_eh() and scsi_restart_operations()
in the case when scsi_restart_operations() issues i/o to other devices
in the sas domain. When this happens the host state transitions from
SHOST_RECOVERY (set by scsi_schedule_eh) back to SHOST_RUNNING and
->host_busy is non-zero so we put the eh thread to sleep even though
->host_eh_scheduled is active.
Before putting the error handler to sleep we need to check if the
host_state needs to return to SHOST_RECOVERY for another trip through
eh.
Cc: Tejun Heo <tj@kernel.org>
Reported-by: Tom Jackson <thomas.p.jackson@intel.com>
Tested-by: Tom Jackson <thomas.p.jackson@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/scsi/scsi_error.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 2cfcbff..0945d47 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -1687,6 +1687,20 @@ static void scsi_restart_operations(struct Scsi_Host *shost)
* requests are started.
*/
scsi_run_host_queues(shost);
+
+ /*
+ * if eh is active and host_eh_scheduled is pending we need to re-run
+ * recovery. we do this check after scsi_run_host_queues() to allow
+ * everything pent up since the last eh run a chance to make forward
+ * progress before we sync again. Either we'll immediately re-run
+ * recovery or scsi_device_unbusy() will wake us again when these
+ * pending commands complete.
+ */
+ spin_lock_irqsave(shost->host_lock, flags);
+ if (shost->host_eh_scheduled)
+ if (scsi_host_set_state(shost, SHOST_RECOVERY))
+ WARN_ON(scsi_host_set_state(shost, SHOST_CANCEL_RECOVERY));
+ spin_unlock_irqrestore(shost->host_lock, flags);
}
/**
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH 10/12] scsi: fix eh wakeup (scsi_schedule_eh vs scsi_restart_operations)
2012-04-13 23:37 ` [PATCH 10/12] scsi: fix eh wakeup (scsi_schedule_eh vs scsi_restart_operations) Dan Williams
@ 2012-04-21 12:22 ` James Bottomley
2012-04-22 15:24 ` Dan Williams
0 siblings, 1 reply; 34+ messages in thread
From: James Bottomley @ 2012-04-21 12:22 UTC (permalink / raw)
To: Dan Williams; +Cc: Tejun Heo, linux-ide, Tom Jackson, linux-scsi
On Fri, 2012-04-13 at 16:37 -0700, Dan Williams wrote:
> Rapid ata hotplug on a libsas controller results in cases where libsas
> is waiting indefinitely on eh to perform an ata probe.
>
> A race exists between scsi_schedule_eh() and scsi_restart_operations()
> in the case when scsi_restart_operations() issues i/o to other devices
> in the sas domain. When this happens the host state transitions from
> SHOST_RECOVERY (set by scsi_schedule_eh) back to SHOST_RUNNING and
> ->host_busy is non-zero so we put the eh thread to sleep even though
> ->host_eh_scheduled is active.
>
> Before putting the error handler to sleep we need to check if the
> host_state needs to return to SHOST_RECOVERY for another trip through
> eh.
>
> Cc: Tejun Heo <tj@kernel.org>
> Reported-by: Tom Jackson <thomas.p.jackson@intel.com>
> Tested-by: Tom Jackson <thomas.p.jackson@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
> drivers/scsi/scsi_error.c | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index 2cfcbff..0945d47 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -1687,6 +1687,20 @@ static void scsi_restart_operations(struct Scsi_Host *shost)
> * requests are started.
> */
> scsi_run_host_queues(shost);
> +
> + /*
> + * if eh is active and host_eh_scheduled is pending we need to re-run
> + * recovery. we do this check after scsi_run_host_queues() to allow
> + * everything pent up since the last eh run a chance to make forward
> + * progress before we sync again. Either we'll immediately re-run
> + * recovery or scsi_device_unbusy() will wake us again when these
> + * pending commands complete.
> + */
> + spin_lock_irqsave(shost->host_lock, flags);
> + if (shost->host_eh_scheduled)
> + if (scsi_host_set_state(shost, SHOST_RECOVERY))
> + WARN_ON(scsi_host_set_state(shost, SHOST_CANCEL_RECOVERY));
> + spin_unlock_irqrestore(shost->host_lock, flags);
This doesn't really look to be the way to fix the race, because we'll
start up the host again before closing it down. Isn't the correct way
to put
if (shost->host_eh_scheduled)
continue;
into the scsi_error_handler() loop just *before*
scsi_restart_operations()?
James
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 10/12] scsi: fix eh wakeup (scsi_schedule_eh vs scsi_restart_operations)
2012-04-21 12:22 ` James Bottomley
@ 2012-04-22 15:24 ` Dan Williams
0 siblings, 0 replies; 34+ messages in thread
From: Dan Williams @ 2012-04-22 15:24 UTC (permalink / raw)
To: James Bottomley; +Cc: Tejun Heo, linux-ide, Tom Jackson, linux-scsi
On Sat, Apr 21, 2012 at 5:22 AM, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
> On Fri, 2012-04-13 at 16:37 -0700, Dan Williams wrote:
>> Rapid ata hotplug on a libsas controller results in cases where libsas
>> is waiting indefinitely on eh to perform an ata probe.
>>
>> A race exists between scsi_schedule_eh() and scsi_restart_operations()
>> in the case when scsi_restart_operations() issues i/o to other devices
>> in the sas domain. When this happens the host state transitions from
>> SHOST_RECOVERY (set by scsi_schedule_eh) back to SHOST_RUNNING and
>> ->host_busy is non-zero so we put the eh thread to sleep even though
>> ->host_eh_scheduled is active.
>>
>> Before putting the error handler to sleep we need to check if the
>> host_state needs to return to SHOST_RECOVERY for another trip through
>> eh.
>>
>> Cc: Tejun Heo <tj@kernel.org>
>> Reported-by: Tom Jackson <thomas.p.jackson@intel.com>
>> Tested-by: Tom Jackson <thomas.p.jackson@intel.com>
>> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>> ---
>> drivers/scsi/scsi_error.c | 14 ++++++++++++++
>> 1 file changed, 14 insertions(+)
>>
>> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
>> index 2cfcbff..0945d47 100644
>> --- a/drivers/scsi/scsi_error.c
>> +++ b/drivers/scsi/scsi_error.c
>> @@ -1687,6 +1687,20 @@ static void scsi_restart_operations(struct Scsi_Host *shost)
>> * requests are started.
>> */
>> scsi_run_host_queues(shost);
>> +
>> + /*
>> + * if eh is active and host_eh_scheduled is pending we need to re-run
>> + * recovery. we do this check after scsi_run_host_queues() to allow
>> + * everything pent up since the last eh run a chance to make forward
>> + * progress before we sync again. Either we'll immediately re-run
>> + * recovery or scsi_device_unbusy() will wake us again when these
>> + * pending commands complete.
>> + */
>> + spin_lock_irqsave(shost->host_lock, flags);
>> + if (shost->host_eh_scheduled)
>> + if (scsi_host_set_state(shost, SHOST_RECOVERY))
>> + WARN_ON(scsi_host_set_state(shost, SHOST_CANCEL_RECOVERY));
>> + spin_unlock_irqrestore(shost->host_lock, flags);
>
> This doesn't really look to be the way to fix the race, because we'll
> start up the host again before closing it down.
That's part of the intent. Any command that has been waiting to be
restarted has been potentially waiting up to a minute for recovery to
complete, so the idea is that we trickle the blocked commands through
and then come back for another pass. This is close to what "late"
callers sto csi_schedule_eh() experience anyways.
> Isn't the correct way to put
>
> if (shost->host_eh_scheduled)
> continue;
>
> into the scsi_error_handler() loop just *before*
> scsi_restart_operations()?
I think that only narrows the window... The hole is
scsi_schedule_eh() called anytime after the last run completes and
before the change of the host state, so host_eh_scheduled can go
active immediately after that check right?
We could move the check into scsi_restart_operations() under the lock
before we change the state back to running, but I figure if we got
that far just let the queue run again.
--
Dan
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 11/12] libsas: fix false positive 'device attached' conditions
2012-04-13 23:36 [GIT PATCH 00/12] libsas fixes for 3.4 Dan Williams
` (9 preceding siblings ...)
2012-04-13 23:37 ` [PATCH 10/12] scsi: fix eh wakeup (scsi_schedule_eh vs scsi_restart_operations) Dan Williams
@ 2012-04-13 23:37 ` Dan Williams
2012-04-22 10:53 ` James Bottomley
2012-04-13 23:37 ` [PATCH 12/12] scsi_transport_sas: fix delete vs scan race Dan Williams
2012-04-14 8:19 ` [GIT PATCH 00/12] libsas fixes for 3.4 jack_wang
12 siblings, 1 reply; 34+ messages in thread
From: Dan Williams @ 2012-04-13 23:37 UTC (permalink / raw)
To: JBottomley; +Cc: linux-ide, Andrzej Jakowski, linux-scsi
Normalize phy->attached_sas_addr to return a zero-address in the case
when device-type == NO_DEVICE or the linkrate is invalid to handle
expanders that put non-zero sas addresses in the discovery response:
sas: ex 5001b4da000f903f phy02:U:0 attached: 0100000000000000 (no device)
sas: ex 5001b4da000f903f phy01:U:0 attached: 0100000000000000 (no device)
sas: ex 5001b4da000f903f phy03:U:0 attached: 0100000000000000 (no device)
sas: ex 5001b4da000f903f phy00:U:0 attached: 0100000000000000 (no device)
Reported-by: Andrzej Jakowski <andrzej.jakowski@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/scsi/libsas/sas_expander.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index 75247a1..caa0525 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -245,7 +245,14 @@ static void sas_set_ex_phy(struct domain_device *dev, int phy_id, void *rsp)
phy->attached_sata_ps = dr->attached_sata_ps;
phy->attached_iproto = dr->iproto << 1;
phy->attached_tproto = dr->tproto << 1;
- memcpy(phy->attached_sas_addr, dr->attached_sas_addr, SAS_ADDR_SIZE);
+ /* help some expanders that fail to zero sas_address in the 'no
+ * device' case
+ */
+ if (phy->attached_dev_type == NO_DEVICE ||
+ phy->linkrate < SAS_LINK_RATE_1_5_GBPS)
+ memset(phy->attached_sas_addr, 0, SAS_ADDR_SIZE);
+ else
+ memcpy(phy->attached_sas_addr, dr->attached_sas_addr, SAS_ADDR_SIZE);
phy->attached_phy_id = dr->attached_phy_id;
phy->phy_change_count = dr->change_count;
phy->routing_attr = dr->routing_attr;
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH 11/12] libsas: fix false positive 'device attached' conditions
2012-04-13 23:37 ` [PATCH 11/12] libsas: fix false positive 'device attached' conditions Dan Williams
@ 2012-04-22 10:53 ` James Bottomley
2012-04-22 15:56 ` Dan Williams
0 siblings, 1 reply; 34+ messages in thread
From: James Bottomley @ 2012-04-22 10:53 UTC (permalink / raw)
To: Dan Williams; +Cc: linux-ide, Andrzej Jakowski, linux-scsi
On Fri, 2012-04-13 at 16:37 -0700, Dan Williams wrote:
> Normalize phy->attached_sas_addr to return a zero-address in the case
> when device-type == NO_DEVICE or the linkrate is invalid to handle
> expanders that put non-zero sas addresses in the discovery response:
>
> sas: ex 5001b4da000f903f phy02:U:0 attached: 0100000000000000 (no device)
> sas: ex 5001b4da000f903f phy01:U:0 attached: 0100000000000000 (no device)
> sas: ex 5001b4da000f903f phy03:U:0 attached: 0100000000000000 (no device)
> sas: ex 5001b4da000f903f phy00:U:0 attached: 0100000000000000 (no device)
>
> Reported-by: Andrzej Jakowski <andrzej.jakowski@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
This should be flagged for stable, shouldn't it?
James
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 11/12] libsas: fix false positive 'device attached' conditions
2012-04-22 10:53 ` James Bottomley
@ 2012-04-22 15:56 ` Dan Williams
0 siblings, 0 replies; 34+ messages in thread
From: Dan Williams @ 2012-04-22 15:56 UTC (permalink / raw)
To: James Bottomley; +Cc: linux-ide, Andrzej Jakowski, linux-scsi
On Sun, Apr 22, 2012 at 3:53 AM, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
> On Fri, 2012-04-13 at 16:37 -0700, Dan Williams wrote:
>> Normalize phy->attached_sas_addr to return a zero-address in the case
>> when device-type == NO_DEVICE or the linkrate is invalid to handle
>> expanders that put non-zero sas addresses in the discovery response:
>>
>> sas: ex 5001b4da000f903f phy02:U:0 attached: 0100000000000000 (no device)
>> sas: ex 5001b4da000f903f phy01:U:0 attached: 0100000000000000 (no device)
>> sas: ex 5001b4da000f903f phy03:U:0 attached: 0100000000000000 (no device)
>> sas: ex 5001b4da000f903f phy00:U:0 attached: 0100000000000000 (no device)
>>
>> Reported-by: Andrzej Jakowski <andrzej.jakowski@intel.com>
>> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>
> This should be flagged for stable, shouldn't it?
Probably should yes... would be nice to not need to rebase to append a
Cc:. So far I have the impression we could handle the review comments
with appending a new commit or two. So If I can keep the commit ids
stable I'll just flag this one to stable@ once it goes upstream.
--
Dan
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 12/12] scsi_transport_sas: fix delete vs scan race
2012-04-13 23:36 [GIT PATCH 00/12] libsas fixes for 3.4 Dan Williams
` (10 preceding siblings ...)
2012-04-13 23:37 ` [PATCH 11/12] libsas: fix false positive 'device attached' conditions Dan Williams
@ 2012-04-13 23:37 ` Dan Williams
2012-04-22 10:38 ` James Bottomley
2012-04-14 8:19 ` [GIT PATCH 00/12] libsas fixes for 3.4 jack_wang
12 siblings, 1 reply; 34+ messages in thread
From: Dan Williams @ 2012-04-13 23:37 UTC (permalink / raw)
To: JBottomley; +Cc: linux-ide, linux-scsi
The following crash results from cases where the end_device has been
removed before scsi_sysfs_add_sdev has had a chance to run.
BUG: unable to handle kernel NULL pointer dereference at 0000000000000098
IP: [<ffffffff8115e100>] sysfs_create_dir+0x32/0xb6
...
Call Trace:
[<ffffffff8125e4a8>] kobject_add_internal+0x120/0x1e3
[<ffffffff81075149>] ? trace_hardirqs_on+0xd/0xf
[<ffffffff8125e641>] kobject_add_varg+0x41/0x50
[<ffffffff8125e70b>] kobject_add+0x64/0x66
[<ffffffff8131122b>] device_add+0x12d/0x63a
[<ffffffff814b65ea>] ? _raw_spin_unlock_irqrestore+0x47/0x56
[<ffffffff8107de15>] ? module_refcount+0x89/0xa0
[<ffffffff8132f348>] scsi_sysfs_add_sdev+0x4e/0x28a
[<ffffffff8132dcbb>] do_scan_async+0x9c/0x145
...teach sas_rphy_remove to wait for async scanning to quiesce before
removing the end_device. It seems this is a more general problem [1],
but this patch only addresses sas transport.
[1]: 23edb6e [SCSI] mpt2sas: Do not set sas_device->starget to NULL from
the slave_destroy callback when all the LUNS have been deleted
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/scsi/scsi_transport_sas.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/scsi/scsi_transport_sas.c b/drivers/scsi/scsi_transport_sas.c
index f7565fc..47abb90 100644
--- a/drivers/scsi/scsi_transport_sas.c
+++ b/drivers/scsi/scsi_transport_sas.c
@@ -33,8 +33,9 @@
#include <linux/bsg.h>
#include <scsi/scsi.h>
-#include <scsi/scsi_device.h>
#include <scsi/scsi_host.h>
+#include <scsi/scsi_scan.h>
+#include <scsi/scsi_device.h>
#include <scsi/scsi_transport.h>
#include <scsi/scsi_transport_sas.h>
@@ -1667,6 +1668,9 @@ sas_rphy_remove(struct sas_rphy *rphy)
{
struct device *dev = &rphy->dev;
+ /* prevent device_del() while child device_add() may be in-flight */
+ scsi_complete_async_scans();
+
switch (rphy->identify.device_type) {
case SAS_END_DEVICE:
scsi_remove_target(dev);
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH 12/12] scsi_transport_sas: fix delete vs scan race
2012-04-13 23:37 ` [PATCH 12/12] scsi_transport_sas: fix delete vs scan race Dan Williams
@ 2012-04-22 10:38 ` James Bottomley
2012-04-22 15:43 ` Dan Williams
0 siblings, 1 reply; 34+ messages in thread
From: James Bottomley @ 2012-04-22 10:38 UTC (permalink / raw)
To: Dan Williams; +Cc: linux-ide, linux-scsi
On Fri, 2012-04-13 at 16:37 -0700, Dan Williams wrote:
> The following crash results from cases where the end_device has been
> removed before scsi_sysfs_add_sdev has had a chance to run.
>
> BUG: unable to handle kernel NULL pointer dereference at 0000000000000098
> IP: [<ffffffff8115e100>] sysfs_create_dir+0x32/0xb6
> ...
> Call Trace:
> [<ffffffff8125e4a8>] kobject_add_internal+0x120/0x1e3
> [<ffffffff81075149>] ? trace_hardirqs_on+0xd/0xf
> [<ffffffff8125e641>] kobject_add_varg+0x41/0x50
> [<ffffffff8125e70b>] kobject_add+0x64/0x66
> [<ffffffff8131122b>] device_add+0x12d/0x63a
> [<ffffffff814b65ea>] ? _raw_spin_unlock_irqrestore+0x47/0x56
> [<ffffffff8107de15>] ? module_refcount+0x89/0xa0
> [<ffffffff8132f348>] scsi_sysfs_add_sdev+0x4e/0x28a
> [<ffffffff8132dcbb>] do_scan_async+0x9c/0x145
>
> ...teach sas_rphy_remove to wait for async scanning to quiesce before
> removing the end_device. It seems this is a more general problem [1],
> but this patch only addresses sas transport.
>
> [1]: 23edb6e [SCSI] mpt2sas: Do not set sas_device->starget to NULL from
> the slave_destroy callback when all the LUNS have been deleted
>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
> drivers/scsi/scsi_transport_sas.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/scsi_transport_sas.c b/drivers/scsi/scsi_transport_sas.c
> index f7565fc..47abb90 100644
> --- a/drivers/scsi/scsi_transport_sas.c
> +++ b/drivers/scsi/scsi_transport_sas.c
> @@ -33,8 +33,9 @@
> #include <linux/bsg.h>
>
> #include <scsi/scsi.h>
> -#include <scsi/scsi_device.h>
> #include <scsi/scsi_host.h>
> +#include <scsi/scsi_scan.h>
> +#include <scsi/scsi_device.h>
> #include <scsi/scsi_transport.h>
> #include <scsi/scsi_transport_sas.h>
>
> @@ -1667,6 +1668,9 @@ sas_rphy_remove(struct sas_rphy *rphy)
> {
> struct device *dev = &rphy->dev;
>
> + /* prevent device_del() while child device_add() may be in-flight */
> + scsi_complete_async_scans();
> +
> switch (rphy->identify.device_type) {
This doesn't really fix the problem, it merely narrows the window (we
still crash in the much shorter window if another async scan starts
after you check for completion). Isn't the fix that will prevent all of
this to hold the scan mutex across scsi_remove_device() ... in which
case it should probably be part of scsi_remove_device()?
James
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH 12/12] scsi_transport_sas: fix delete vs scan race
2012-04-22 10:38 ` James Bottomley
@ 2012-04-22 15:43 ` Dan Williams
2012-04-22 17:15 ` James Bottomley
0 siblings, 1 reply; 34+ messages in thread
From: Dan Williams @ 2012-04-22 15:43 UTC (permalink / raw)
To: James Bottomley; +Cc: linux-ide, linux-scsi
On Sun, Apr 22, 2012 at 3:38 AM, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
> On Fri, 2012-04-13 at 16:37 -0700, Dan Williams wrote:
>> The following crash results from cases where the end_device has been
>> removed before scsi_sysfs_add_sdev has had a chance to run.
>>
>> BUG: unable to handle kernel NULL pointer dereference at 0000000000000098
>> IP: [<ffffffff8115e100>] sysfs_create_dir+0x32/0xb6
>> ...
>> Call Trace:
>> [<ffffffff8125e4a8>] kobject_add_internal+0x120/0x1e3
>> [<ffffffff81075149>] ? trace_hardirqs_on+0xd/0xf
>> [<ffffffff8125e641>] kobject_add_varg+0x41/0x50
>> [<ffffffff8125e70b>] kobject_add+0x64/0x66
>> [<ffffffff8131122b>] device_add+0x12d/0x63a
>> [<ffffffff814b65ea>] ? _raw_spin_unlock_irqrestore+0x47/0x56
>> [<ffffffff8107de15>] ? module_refcount+0x89/0xa0
>> [<ffffffff8132f348>] scsi_sysfs_add_sdev+0x4e/0x28a
>> [<ffffffff8132dcbb>] do_scan_async+0x9c/0x145
>>
>> ...teach sas_rphy_remove to wait for async scanning to quiesce before
>> removing the end_device. It seems this is a more general problem [1],
>> but this patch only addresses sas transport.
>>
>> [1]: 23edb6e [SCSI] mpt2sas: Do not set sas_device->starget to NULL from
>> the slave_destroy callback when all the LUNS have been deleted
>>
>> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>> ---
>> drivers/scsi/scsi_transport_sas.c | 6 +++++-
>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/scsi/scsi_transport_sas.c b/drivers/scsi/scsi_transport_sas.c
>> index f7565fc..47abb90 100644
>> --- a/drivers/scsi/scsi_transport_sas.c
>> +++ b/drivers/scsi/scsi_transport_sas.c
>> @@ -33,8 +33,9 @@
>> #include <linux/bsg.h>
>>
>> #include <scsi/scsi.h>
>> -#include <scsi/scsi_device.h>
>> #include <scsi/scsi_host.h>
>> +#include <scsi/scsi_scan.h>
>> +#include <scsi/scsi_device.h>
>> #include <scsi/scsi_transport.h>
>> #include <scsi/scsi_transport_sas.h>
>>
>> @@ -1667,6 +1668,9 @@ sas_rphy_remove(struct sas_rphy *rphy)
>> {
>> struct device *dev = &rphy->dev;
>>
>> + /* prevent device_del() while child device_add() may be in-flight */
>> + scsi_complete_async_scans();
>> +
>> switch (rphy->identify.device_type) {
>
> This doesn't really fix the problem, it merely narrows the window (we
> still crash in the much shorter window if another async scan starts
> after you check for completion).
Oh, I was under the impression that async scanning was only the
initial scan and everything was sync thereafter since
scsi_finish_async_scan() clears the host ->async_scan flag?
> Isn't the fix that will prevent all of
> this to hold the scan mutex across scsi_remove_device() ... in which
> case it should probably be part of scsi_remove_device()?
I thought along these lines initially, but in this case we're crashing
because the sas rphy is removed before the starget is added, so
scsi_remove_device() is out of the picture.
--
Dan
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH 12/12] scsi_transport_sas: fix delete vs scan race
2012-04-22 15:43 ` Dan Williams
@ 2012-04-22 17:15 ` James Bottomley
2012-05-05 21:52 ` Dan Williams
0 siblings, 1 reply; 34+ messages in thread
From: James Bottomley @ 2012-04-22 17:15 UTC (permalink / raw)
To: Dan Williams; +Cc: linux-ide, linux-scsi
On Sun, 2012-04-22 at 08:43 -0700, Dan Williams wrote:
> On Sun, Apr 22, 2012 at 3:38 AM, James Bottomley
> <James.Bottomley@hansenpartnership.com> wrote:
> > On Fri, 2012-04-13 at 16:37 -0700, Dan Williams wrote:
> >> The following crash results from cases where the end_device has been
> >> removed before scsi_sysfs_add_sdev has had a chance to run.
> >>
> >> BUG: unable to handle kernel NULL pointer dereference at 0000000000000098
> >> IP: [<ffffffff8115e100>] sysfs_create_dir+0x32/0xb6
> >> ...
> >> Call Trace:
> >> [<ffffffff8125e4a8>] kobject_add_internal+0x120/0x1e3
> >> [<ffffffff81075149>] ? trace_hardirqs_on+0xd/0xf
> >> [<ffffffff8125e641>] kobject_add_varg+0x41/0x50
> >> [<ffffffff8125e70b>] kobject_add+0x64/0x66
> >> [<ffffffff8131122b>] device_add+0x12d/0x63a
> >> [<ffffffff814b65ea>] ? _raw_spin_unlock_irqrestore+0x47/0x56
> >> [<ffffffff8107de15>] ? module_refcount+0x89/0xa0
> >> [<ffffffff8132f348>] scsi_sysfs_add_sdev+0x4e/0x28a
> >> [<ffffffff8132dcbb>] do_scan_async+0x9c/0x145
> >>
> >> ...teach sas_rphy_remove to wait for async scanning to quiesce before
> >> removing the end_device. It seems this is a more general problem [1],
> >> but this patch only addresses sas transport.
> >>
> >> [1]: 23edb6e [SCSI] mpt2sas: Do not set sas_device->starget to NULL from
> >> the slave_destroy callback when all the LUNS have been deleted
> >>
> >> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> >> ---
> >> drivers/scsi/scsi_transport_sas.c | 6 +++++-
> >> 1 file changed, 5 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/scsi/scsi_transport_sas.c b/drivers/scsi/scsi_transport_sas.c
> >> index f7565fc..47abb90 100644
> >> --- a/drivers/scsi/scsi_transport_sas.c
> >> +++ b/drivers/scsi/scsi_transport_sas.c
> >> @@ -33,8 +33,9 @@
> >> #include <linux/bsg.h>
> >>
> >> #include <scsi/scsi.h>
> >> -#include <scsi/scsi_device.h>
> >> #include <scsi/scsi_host.h>
> >> +#include <scsi/scsi_scan.h>
> >> +#include <scsi/scsi_device.h>
> >> #include <scsi/scsi_transport.h>
> >> #include <scsi/scsi_transport_sas.h>
> >>
> >> @@ -1667,6 +1668,9 @@ sas_rphy_remove(struct sas_rphy *rphy)
> >> {
> >> struct device *dev = &rphy->dev;
> >>
> >> + /* prevent device_del() while child device_add() may be in-flight */
> >> + scsi_complete_async_scans();
> >> +
> >> switch (rphy->identify.device_type) {
> >
> > This doesn't really fix the problem, it merely narrows the window (we
> > still crash in the much shorter window if another async scan starts
> > after you check for completion).
>
> Oh, I was under the impression that async scanning was only the
> initial scan and everything was sync thereafter since
> scsi_finish_async_scan() clears the host ->async_scan flag?
Async scan here means any scan in a different thread, right ... it just
has to be asynchronous relative to us? So that includes the manually
initiated ones and hotplug ones, doesn't it?
> > Isn't the fix that will prevent all of
> > this to hold the scan mutex across scsi_remove_device() ... in which
> > case it should probably be part of scsi_remove_device()?
>
> I thought along these lines initially, but in this case we're crashing
> because the sas rphy is removed before the starget is added, so
> scsi_remove_device() is out of the picture.
Just adding the sequence
mutex_lock(&shost->scan_mutex);
mutex_unlock(&shost->scan_mutex);
is logically a subset of
scsi_complete_async_scans()
So putting it here:
diff --git a/drivers/scsi/scsi_transport_sas.c b/drivers/scsi/scsi_transport_sas.c
index f7565fc..c89bba6 100644
--- a/drivers/scsi/scsi_transport_sas.c
+++ b/drivers/scsi/scsi_transport_sas.c
@@ -1669,7 +1669,9 @@ sas_rphy_remove(struct sas_rphy *rphy)
switch (rphy->identify.device_type) {
case SAS_END_DEVICE:
+ mutex_lock(&shost->scan_mutex);
scsi_remove_target(dev);
+ mutex_unlock(&shost->scan_mutex);
break;
case SAS_EDGE_EXPANDER_DEVICE:
case SAS_FANOUT_EXPANDER_DEVICE:
should definitely be equivalent to scsi_complete_async_scans() above the
switch statement. The questions are a) should it be inside
scsi_remove_target() because that seems to be the sync point and b) does
it fix all the races.
James
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH 12/12] scsi_transport_sas: fix delete vs scan race
2012-04-22 17:15 ` James Bottomley
@ 2012-05-05 21:52 ` Dan Williams
2012-05-20 19:20 ` Dan Williams
0 siblings, 1 reply; 34+ messages in thread
From: Dan Williams @ 2012-05-05 21:52 UTC (permalink / raw)
To: James Bottomley; +Cc: linux-ide, linux-scsi
On Sun, Apr 22, 2012 at 10:15 AM, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
> Async scan here means any scan in a different thread, right ... it just
> has to be asynchronous relative to us? So that includes the manually
> initiated ones and hotplug ones, doesn't it?
[ resend since I notice this never hit the lists ]
Hmm, well no I don't think so. This literally means the initial async
scan, and the
failure window is between when we skip the call to
scsi_sysfs_add_sdev() (in scsi_add_lun() under the scan_mutex) and
finally call scsi_sysfs_add_sdev() again via scsi_finish_async_scan().
I don't see how that fixes it because when we fail the sequence goes:
mutex_lock(scan_mutex)
starget->parent = end_device;
scsi_add_lun()
mutex_unlock(scan_mutex)
device_del(end_device)
mutex_lock(scan_mutex)
device_add(starget)
<crash>
As far as I can see taking the scan_mutex in sas_rphy_remove() does
not change this failure window. Unless I missed something?
I am going to re-submit this patch as is with the proposed libsas batch for 3.5.
--
Dan
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 12/12] scsi_transport_sas: fix delete vs scan race
2012-05-05 21:52 ` Dan Williams
@ 2012-05-20 19:20 ` Dan Williams
0 siblings, 0 replies; 34+ messages in thread
From: Dan Williams @ 2012-05-20 19:20 UTC (permalink / raw)
To: James Bottomley; +Cc: linux-ide, linux-scsi, Dariusz Majchrzak
On Sat, May 5, 2012 at 2:52 PM, Dan Williams <dan.j.williams@intel.com> wrote:
> On Sun, Apr 22, 2012 at 10:15 AM, James Bottomley
> <James.Bottomley@hansenpartnership.com> wrote:
>> Async scan here means any scan in a different thread, right ... it just
>> has to be asynchronous relative to us? So that includes the manually
>> initiated ones and hotplug ones, doesn't it?
>
> [ resend since I notice this never hit the lists ]
>
> Hmm, well no I don't think so. This literally means the initial async
> scan, and the
> failure window is between when we skip the call to
> scsi_sysfs_add_sdev() (in scsi_add_lun() under the scan_mutex) and
> finally call scsi_sysfs_add_sdev() again via scsi_finish_async_scan().
> I don't see how that fixes it because when we fail the sequence goes:
>
> mutex_lock(scan_mutex)
> starget->parent = end_device;
> scsi_add_lun()
> mutex_unlock(scan_mutex)
>
> device_del(end_device)
>
> mutex_lock(scan_mutex)
> device_add(starget)
> <crash>
>
> As far as I can see taking the scan_mutex in sas_rphy_remove() does
> not change this failure window. Unless I missed something?
>
> I am going to re-submit this patch as is with the proposed libsas batch for 3.5.
It turns out this patch can cause a deadlock in the scenario where we
have two hosts scanning and the "previous" host (according to the
async scan queue), experiences a device removal event. I think the
following should be all we need:
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 01b0374..8906557 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -1714,6 +1714,9 @@ static void scsi_sysfs_add_devices(struct
Scsi_Host *shost)
{
struct scsi_device *sdev;
shost_for_each_device(sdev, shost) {
+ /* target removed before the device could be added */
+ if (sdev->sdev_state == SDEV_DEL)
+ continue;
if (!scsi_host_scan_allowed(shost) ||
scsi_sysfs_add_sdev(sdev) != 0)
__scsi_remove_device(sdev);
...since starget removal will mark the sdevs as deleted under
scan_mutex. scsi_sysfs_add_devices can simply ignore deleted devices.
I'll post this patch after Darek has a chance to try it out.
--
Dan
^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [GIT PATCH 00/12] libsas fixes for 3.4
2012-04-13 23:36 [GIT PATCH 00/12] libsas fixes for 3.4 Dan Williams
` (11 preceding siblings ...)
2012-04-13 23:37 ` [PATCH 12/12] scsi_transport_sas: fix delete vs scan race Dan Williams
@ 2012-04-14 8:19 ` jack_wang
12 siblings, 0 replies; 34+ messages in thread
From: jack_wang @ 2012-04-14 8:19 UTC (permalink / raw)
To: Dan Williams, James Bottomley; +Cc: linux-ide, linux-scsi
Hi Dan,
Thanks for doing this. I've tested this patchset for a while and it works good.
You can add my test-by or acked-by if needed.
Best regards!
--------------
jack_wang
>The following changes since commit cd8df932d894f3128c884e3ae1b2b484540513db:
>
> [SCSI] qla4xxx: Update driver version to 5.02.00-k15 (2012-02-29 17:03:03 -0600)
>
>are available in the git repository at:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/djbw/isci.git tags/libsas-fixes
>
>for you to fetch changes up to e81dcce46fdbb2c968d7314c2f19da3c2bba24d1:
>
> scsi_transport_sas: fix delete vs scan race (2012-04-10 19:15:23 -0700)
>
>----------------------------------------------------------------
>libsas-fixes for 3.4-rc4
>
>Regression fixes to stabilize the new workqueue and ata asynchronous
>error handling implementations that were merged for v3.4-rc1.
>
>1/ fix sas_drain_work() which was stomping on 'work' entries while the
> workqueue was manipulating them. user would see random crashes when
> trying to use scsi_transport_sas attributes for resets, or during
> discovery
>
>2/ (2) longstanding bugs related to the fact that libata (inventor and
> primary host_eh_scheduled user) had built-in assumptions of 1:1
> Scsi_Host-to-ata_port relationship. The libsas 1:N arrangement
> magnified these problems when it gained async eh and began scheduling
> eh in more scenarios (sas-transports resets) in 3.4-rc1.
>
>3/ lifetime fixes for the rphy since code that has a domain_device
> reference expected to be able to de-reference rphy parameters.
>
>4/ (3) fixes for expander discovery bugs, one a recent regression with
> ata-eh clobbering expander-phy data as it polled leading to system
> crashes, a long standing bug that caused libsas to be
> incompatible with expanders that advertised "PHY_VACANT" in low order
> phy indexes, and a quirk for expanders that sometimes fail to zero
> the sas address when no device is attached.
>
>5/ fix for a long-standing bug whereby hotunplug events during initial
> host scan can cause a system crash
>
>----------------------------------------------------------------
>
>This is a reflow of the 26 patches in the libsas-eh-reworks-v15 branch
>to separate out the 12 fixes from the other feature development. These
>patches, save for the new "scsi: fix eh wakeup (scsi_schedule_eh vs
>scsi_restart_operations)", were all originally posted before the merge
>window opened, and have also appeared in -next for the same timeframe.
>
>There is a mix of pure regression fixes and fixes for long-standing bugs
>in libsas. Some of the long-standing bug fixes are made worse / easier
>to trigger by the new async error handling scheme.
>
>The largest patch in the series is "libata, libsas: introduce sched_eh
>and end_eh port ops" wants an ack from the ata folks, it has been on the
>list since March 10th.
>
>Dan Williams (10):
> libsas: introduce sas_work to fix sas_drain_work vs sas_queue_work
> libata, libsas: introduce sched_eh and end_eh port ops
> libsas: fix sas_get_port_device regression
> libsas: unify domain_device sas_rphy lifetimes
> libsas: fix ata_eh clobbering ex_phys via smp_ata_check_ready
> libata: make ata_print_id atomic
> libsas, libata: fix start of life for a sas ata_port
> scsi: fix eh wakeup (scsi_schedule_eh vs scsi_restart_operations)
> libsas: fix false positive 'device attached' conditions
> scsi_transport_sas: fix delete vs scan race
>
>Maciej Trela (1):
> libsas: cleanup spurious calls to scsi_schedule_eh
>
>Thomas Jackson (1):
> libsas: fix sas_find_bcast_phy() in the presence of 'vacant' phys
>
> drivers/ata/libata-core.c | 8 +++-
> drivers/ata/libata-eh.c | 57 +++++++++++++++++++++------
> drivers/ata/libata-scsi.c | 35 +++++++++--------
> drivers/ata/libata.h | 2 +-
> drivers/scsi/ipr.c | 6 ++-
> drivers/scsi/libsas/sas_ata.c | 72 +++++++++++++++++++++--------------
> drivers/scsi/libsas/sas_discover.c | 67 ++++++++++++++++++--------------
> drivers/scsi/libsas/sas_event.c | 36 +++++++++---------
> drivers/scsi/libsas/sas_expander.c | 56 +++++++++++++++++++++------
> drivers/scsi/libsas/sas_init.c | 25 ++++++------
> drivers/scsi/libsas/sas_internal.h | 6 +--
> drivers/scsi/libsas/sas_phy.c | 21 ++++------
> drivers/scsi/libsas/sas_port.c | 15 +++-----
> drivers/scsi/libsas/sas_scsi_host.c | 28 ++++++++++----
> drivers/scsi/scsi_error.c | 14 +++++++
> drivers/scsi/scsi_transport_sas.c | 6 ++-
> include/linux/libata.h | 7 +++-
> include/scsi/libsas.h | 44 ++++++++++++++++++---
> include/scsi/sas_ata.h | 9 ++++-
> 19 files changed, 343 insertions(+), 171 deletions(-)
>--
>To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>__________ Information from ESET NOD32 Antivirus, version of virus signature database 5659 (20101129) __________
>
>The message was checked by ESET NOD32 Antivirus.
>
>http://www.eset.com
>
>
>
^ permalink raw reply [flat|nested] 34+ messages in thread