* [PATCH 01/24] workqueue: promote workqueue_lock to hard-irq safe
2011-12-17 2:33 [PATCH 00/24] libsas: eh reworks (ata-eh vs discovery, races, ...) Dan Williams
@ 2011-12-17 2:33 ` Dan Williams
2011-12-17 2:33 ` [PATCH 02/24] workqueue: defer work to a draining queue Dan Williams
` (22 subsequent siblings)
23 siblings, 0 replies; 39+ messages in thread
From: Dan Williams @ 2011-12-17 2:33 UTC (permalink / raw)
To: linux-scsi; +Cc: linux-ide
In preparation for a deferred work implementation to queue unchained
work at the conclusion of a drain_workqueue() event.
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
kernel/workqueue.c | 44 ++++++++++++++++++++++----------------------
1 files changed, 22 insertions(+), 22 deletions(-)
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 42fa9ad..247c59d 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -2403,10 +2403,10 @@ void drain_workqueue(struct workqueue_struct *wq)
* hotter than drain_workqueue() and already looks at @wq->flags.
* Use WQ_DRAINING so that queue doesn't have to check nr_drainers.
*/
- spin_lock(&workqueue_lock);
+ spin_lock_irq(&workqueue_lock);
if (!wq->nr_drainers++)
wq->flags |= WQ_DRAINING;
- spin_unlock(&workqueue_lock);
+ spin_unlock_irq(&workqueue_lock);
reflush:
flush_workqueue(wq);
@@ -2428,10 +2428,10 @@ reflush:
goto reflush;
}
- spin_lock(&workqueue_lock);
+ spin_lock_irq(&workqueue_lock);
if (!--wq->nr_drainers)
wq->flags &= ~WQ_DRAINING;
- spin_unlock(&workqueue_lock);
+ spin_unlock_irq(&workqueue_lock);
}
EXPORT_SYMBOL_GPL(drain_workqueue);
@@ -3033,7 +3033,7 @@ struct workqueue_struct *__alloc_workqueue_key(const char *name,
* list. Grab it, set max_active accordingly and add the new
* workqueue to workqueues list.
*/
- spin_lock(&workqueue_lock);
+ spin_lock_irq(&workqueue_lock);
if (workqueue_freezing && wq->flags & WQ_FREEZABLE)
for_each_cwq_cpu(cpu, wq)
@@ -3041,7 +3041,7 @@ struct workqueue_struct *__alloc_workqueue_key(const char *name,
list_add(&wq->list, &workqueues);
- spin_unlock(&workqueue_lock);
+ spin_unlock_irq(&workqueue_lock);
return wq;
err:
@@ -3072,9 +3072,9 @@ void destroy_workqueue(struct workqueue_struct *wq)
* wq list is used to freeze wq, remove from list after
* flushing is complete in case freeze races us.
*/
- spin_lock(&workqueue_lock);
+ spin_lock_irq(&workqueue_lock);
list_del(&wq->list);
- spin_unlock(&workqueue_lock);
+ spin_unlock_irq(&workqueue_lock);
/* sanity check */
for_each_cwq_cpu(cpu, wq) {
@@ -3114,23 +3114,23 @@ void workqueue_set_max_active(struct workqueue_struct *wq, int max_active)
max_active = wq_clamp_max_active(max_active, wq->flags, wq->name);
- spin_lock(&workqueue_lock);
+ spin_lock_irq(&workqueue_lock);
wq->saved_max_active = max_active;
for_each_cwq_cpu(cpu, wq) {
struct global_cwq *gcwq = get_gcwq(cpu);
- spin_lock_irq(&gcwq->lock);
+ spin_lock(&gcwq->lock);
if (!(wq->flags & WQ_FREEZABLE) ||
!(gcwq->flags & GCWQ_FREEZING))
get_cwq(gcwq->cpu, wq)->max_active = max_active;
- spin_unlock_irq(&gcwq->lock);
+ spin_unlock(&gcwq->lock);
}
- spin_unlock(&workqueue_lock);
+ spin_unlock_irq(&workqueue_lock);
}
EXPORT_SYMBOL_GPL(workqueue_set_max_active);
@@ -3642,7 +3642,7 @@ void freeze_workqueues_begin(void)
{
unsigned int cpu;
- spin_lock(&workqueue_lock);
+ spin_lock_irq(&workqueue_lock);
BUG_ON(workqueue_freezing);
workqueue_freezing = true;
@@ -3651,7 +3651,7 @@ void freeze_workqueues_begin(void)
struct global_cwq *gcwq = get_gcwq(cpu);
struct workqueue_struct *wq;
- spin_lock_irq(&gcwq->lock);
+ spin_lock(&gcwq->lock);
BUG_ON(gcwq->flags & GCWQ_FREEZING);
gcwq->flags |= GCWQ_FREEZING;
@@ -3663,10 +3663,10 @@ void freeze_workqueues_begin(void)
cwq->max_active = 0;
}
- spin_unlock_irq(&gcwq->lock);
+ spin_unlock(&gcwq->lock);
}
- spin_unlock(&workqueue_lock);
+ spin_unlock_irq(&workqueue_lock);
}
/**
@@ -3687,7 +3687,7 @@ bool freeze_workqueues_busy(void)
unsigned int cpu;
bool busy = false;
- spin_lock(&workqueue_lock);
+ spin_lock_irq(&workqueue_lock);
BUG_ON(!workqueue_freezing);
@@ -3711,7 +3711,7 @@ bool freeze_workqueues_busy(void)
}
}
out_unlock:
- spin_unlock(&workqueue_lock);
+ spin_unlock_irq(&workqueue_lock);
return busy;
}
@@ -3728,7 +3728,7 @@ void thaw_workqueues(void)
{
unsigned int cpu;
- spin_lock(&workqueue_lock);
+ spin_lock_irq(&workqueue_lock);
if (!workqueue_freezing)
goto out_unlock;
@@ -3737,7 +3737,7 @@ void thaw_workqueues(void)
struct global_cwq *gcwq = get_gcwq(cpu);
struct workqueue_struct *wq;
- spin_lock_irq(&gcwq->lock);
+ spin_lock(&gcwq->lock);
BUG_ON(!(gcwq->flags & GCWQ_FREEZING));
gcwq->flags &= ~GCWQ_FREEZING;
@@ -3758,12 +3758,12 @@ void thaw_workqueues(void)
wake_up_worker(gcwq);
- spin_unlock_irq(&gcwq->lock);
+ spin_unlock(&gcwq->lock);
}
workqueue_freezing = false;
out_unlock:
- spin_unlock(&workqueue_lock);
+ spin_unlock_irq(&workqueue_lock);
}
#endif /* CONFIG_FREEZER */
^ permalink raw reply related [flat|nested] 39+ messages in thread* [PATCH 02/24] workqueue: defer work to a draining queue
2011-12-17 2:33 [PATCH 00/24] libsas: eh reworks (ata-eh vs discovery, races, ...) Dan Williams
2011-12-17 2:33 ` [PATCH 01/24] workqueue: promote workqueue_lock to hard-irq safe Dan Williams
@ 2011-12-17 2:33 ` Dan Williams
2011-12-19 21:49 ` Tejun Heo
2011-12-17 2:33 ` [PATCH 03/24] scsi: use drain_workqueue Dan Williams
` (21 subsequent siblings)
23 siblings, 1 reply; 39+ messages in thread
From: Dan Williams @ 2011-12-17 2:33 UTC (permalink / raw)
To: linux-scsi; +Cc: Tejun Heo, linux-ide
commit 9c5a2ba7 "workqueue: separate out drain_workqueue() from
destroy_workqueue()" provided drain_workqueue() for users like libsas to
use for flushing events.
When libsas drains it wants currently queued and chained events to be
flushed, but it fully expects to continue issuing unchained events with
the expectation that they are serviced sometime after the drain.
For external users of drain_workqueue() arrange for unchained work to be
queued after the drain completes, if the caller cares if unchained work
was queued as a result of the drain it can check for a non-zero return
value. Deferred work is guaranteed to be at least queued when
drain_workqueue() returns, and visible to flush_workqueue() users as
well.
Unfortunately this causes the promotion of workqueue_lock to hard-irq
safe and does not guarantee that work submitted via queue_work_on() runs
on the specified cpu if it gets deferred.
Cc: Tejun Heo <tj@kernel.org>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
include/linux/workqueue.h | 3 +
kernel/workqueue.c | 97 ++++++++++++++++++++++++++++++++++++---------
2 files changed, 79 insertions(+), 21 deletions(-)
diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
index 0d556de..37de207 100644
--- a/include/linux/workqueue.h
+++ b/include/linux/workqueue.h
@@ -257,6 +257,7 @@ enum {
WQ_DRAINING = 1 << 6, /* internal: workqueue is draining */
WQ_RESCUER = 1 << 7, /* internal: workqueue has rescuer */
+ WQ_NO_DEFER = 1 << 8, /* internal: workqueue destructing */
WQ_MAX_ACTIVE = 512, /* I like 512, better ideas? */
WQ_MAX_UNBOUND_PER_CPU = 4, /* 4 * #cpus for unbound wq */
@@ -355,7 +356,7 @@ extern int queue_delayed_work_on(int cpu, struct workqueue_struct *wq,
struct delayed_work *work, unsigned long delay);
extern void flush_workqueue(struct workqueue_struct *wq);
-extern void drain_workqueue(struct workqueue_struct *wq);
+extern int drain_workqueue(struct workqueue_struct *wq);
extern void flush_scheduled_work(void);
extern int schedule_work(struct work_struct *work);
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 247c59d..fc4687a 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -236,11 +236,12 @@ struct workqueue_struct {
struct wq_flusher *first_flusher; /* F: first flusher */
struct list_head flusher_queue; /* F: flush waiters */
struct list_head flusher_overflow; /* F: flush overflow list */
+ struct mutex drain_mutex; /* 1 drainer at a time */
+ struct list_head drain_defer; /* W: unchained work to defer */
mayday_mask_t mayday_mask; /* cpus requesting rescue */
struct worker *rescuer; /* I: rescue worker */
- int nr_drainers; /* W: drain in progress */
int saved_max_active; /* W: saved cwq max_active */
const char *name; /* I: workqueue name */
#ifdef CONFIG_LOCKDEP
@@ -979,6 +980,19 @@ static bool is_chained_work(struct workqueue_struct *wq)
return false;
}
+static bool defer_work(struct workqueue_struct *wq, struct work_struct *work)
+{
+ if (is_chained_work(wq))
+ return false;
+
+ if (WARN_ON_ONCE(wq->flags & WQ_NO_DEFER))
+ return true;
+
+ list_add_tail(&work->entry, &wq->drain_defer);
+
+ return true;
+}
+
static void __queue_work(unsigned int cpu, struct workqueue_struct *wq,
struct work_struct *work)
{
@@ -991,9 +1005,17 @@ static void __queue_work(unsigned int cpu, struct workqueue_struct *wq,
debug_work_activate(work);
/* if dying, only works from the same workqueue are allowed */
- if (unlikely(wq->flags & WQ_DRAINING) &&
- WARN_ON_ONCE(!is_chained_work(wq)))
- return;
+ if (unlikely(wq->flags & WQ_DRAINING)) {
+ unsigned long flags;
+ bool defer = false;
+
+ spin_lock_irqsave(&workqueue_lock, flags);
+ if (wq->flags & WQ_DRAINING)
+ defer = defer_work(wq, work);
+ spin_unlock_irqrestore(&workqueue_lock, flags);
+ if (defer)
+ return;
+ }
/* determine gcwq to use */
if (!(wq->flags & WQ_UNBOUND)) {
@@ -2227,7 +2249,7 @@ static bool flush_workqueue_prep_cwqs(struct workqueue_struct *wq,
}
/**
- * flush_workqueue - ensure that any scheduled work has run to completion.
+ * __flush_workqueue - ensure that any scheduled work has run to completion.
* @wq: workqueue to flush
*
* Forces execution of the workqueue and blocks until its completion.
@@ -2236,7 +2258,7 @@ static bool flush_workqueue_prep_cwqs(struct workqueue_struct *wq,
* We sleep until all works which were queued on entry have been handled,
* but we are not livelocked by new incoming ones.
*/
-void flush_workqueue(struct workqueue_struct *wq)
+static void __flush_workqueue(struct workqueue_struct *wq)
{
struct wq_flusher this_flusher = {
.list = LIST_HEAD_INIT(this_flusher.list),
@@ -2380,11 +2402,20 @@ void flush_workqueue(struct workqueue_struct *wq)
out_unlock:
mutex_unlock(&wq->flush_mutex);
}
+
+
+void flush_workqueue(struct workqueue_struct *wq)
+{
+ mutex_lock(&wq->drain_mutex);
+ __flush_workqueue(wq);
+ mutex_unlock(&wq->drain_mutex);
+}
EXPORT_SYMBOL_GPL(flush_workqueue);
/**
- * drain_workqueue - drain a workqueue
+ * __drain_workqueue - drain a workqueue
* @wq: workqueue to drain
+ * @flags: WQ_NO_DEFER - reject unchained work
*
* Wait until the workqueue becomes empty. While draining is in progress,
* only chain queueing is allowed. IOW, only currently pending or running
@@ -2392,23 +2423,25 @@ EXPORT_SYMBOL_GPL(flush_workqueue);
* repeatedly until it becomes empty. The number of flushing is detemined
* by the depth of chaining and should be relatively short. Whine if it
* takes too long.
+ *
+ * Indicate to the caller if any deferred (unchained) work was queued
+ * during the drain.
*/
-void drain_workqueue(struct workqueue_struct *wq)
+static int __drain_workqueue(struct workqueue_struct *wq, int flags)
{
+ struct work_struct *work, *w;
unsigned int flush_cnt = 0;
+ LIST_HEAD(drain_defer);
unsigned int cpu;
+ int ret = 0;
+
+ mutex_lock(&wq->drain_mutex);
- /*
- * __queue_work() needs to test whether there are drainers, is much
- * hotter than drain_workqueue() and already looks at @wq->flags.
- * Use WQ_DRAINING so that queue doesn't have to check nr_drainers.
- */
spin_lock_irq(&workqueue_lock);
- if (!wq->nr_drainers++)
- wq->flags |= WQ_DRAINING;
+ wq->flags |= WQ_DRAINING | flags;
spin_unlock_irq(&workqueue_lock);
reflush:
- flush_workqueue(wq);
+ __flush_workqueue(wq);
for_each_cwq_cpu(cpu, wq) {
struct cpu_workqueue_struct *cwq = get_cwq(cpu, wq);
@@ -2429,9 +2462,27 @@ reflush:
}
spin_lock_irq(&workqueue_lock);
- if (!--wq->nr_drainers)
- wq->flags &= ~WQ_DRAINING;
+ wq->flags &= ~(WQ_DRAINING | WQ_NO_DEFER);
+ list_splice_init(&wq->drain_defer, &drain_defer);
+ ret = !list_empty(&drain_defer);
spin_unlock_irq(&workqueue_lock);
+
+ /* submit deferred work provided wq was not being destroyed */
+ list_for_each_entry_safe(work, w, &drain_defer, entry) {
+ list_del_init(&work->entry);
+ queue_work(wq, work);
+ }
+
+ mutex_unlock(&wq->drain_mutex);
+
+ return ret;
+}
+
+int drain_workqueue(struct workqueue_struct *wq)
+{
+ if (WARN_ON_ONCE(wq->flags & WQ_NO_DEFER))
+ return 0; /* lost drain vs destroy race */
+ return __drain_workqueue(wq, 0);
}
EXPORT_SYMBOL_GPL(drain_workqueue);
@@ -2987,9 +3038,11 @@ struct workqueue_struct *__alloc_workqueue_key(const char *name,
wq->flags = flags;
wq->saved_max_active = max_active;
mutex_init(&wq->flush_mutex);
+ mutex_init(&wq->drain_mutex);
atomic_set(&wq->nr_cwqs_to_flush, 0);
INIT_LIST_HEAD(&wq->flusher_queue);
INIT_LIST_HEAD(&wq->flusher_overflow);
+ INIT_LIST_HEAD(&wq->drain_defer);
wq->name = name;
lockdep_init_map(&wq->lockdep_map, lock_name, key, 0);
@@ -3065,8 +3118,12 @@ void destroy_workqueue(struct workqueue_struct *wq)
{
unsigned int cpu;
- /* drain it before proceeding with destruction */
- drain_workqueue(wq);
+ /*
+ * drain it before proceeding with destruction and disable drain
+ * deferrement. !is_chained_work() that arrives after this
+ * point will be dropped on the floor
+ */
+ __drain_workqueue(wq, WQ_NO_DEFER);
/*
* wq list is used to freeze wq, remove from list after
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH 02/24] workqueue: defer work to a draining queue
2011-12-17 2:33 ` [PATCH 02/24] workqueue: defer work to a draining queue Dan Williams
@ 2011-12-19 21:49 ` Tejun Heo
2011-12-19 22:01 ` Dan Williams
0 siblings, 1 reply; 39+ messages in thread
From: Tejun Heo @ 2011-12-19 21:49 UTC (permalink / raw)
To: Dan Williams; +Cc: linux-scsi, linux-ide
Hello,
On Fri, Dec 16, 2011 at 06:33:18PM -0800, Dan Williams wrote:
> commit 9c5a2ba7 "workqueue: separate out drain_workqueue() from
> destroy_workqueue()" provided drain_workqueue() for users like libsas to
> use for flushing events.
>
> When libsas drains it wants currently queued and chained events to be
> flushed, but it fully expects to continue issuing unchained events with
> the expectation that they are serviced sometime after the drain.
>
> For external users of drain_workqueue() arrange for unchained work to be
> queued after the drain completes, if the caller cares if unchained work
> was queued as a result of the drain it can check for a non-zero return
> value. Deferred work is guaranteed to be at least queued when
> drain_workqueue() returns, and visible to flush_workqueue() users as
> well.
>
> Unfortunately this causes the promotion of workqueue_lock to hard-irq
> safe and does not guarantee that work submitted via queue_work_on() runs
> on the specified cpu if it gets deferred.
>
> Cc: Tejun Heo <tj@kernel.org>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Dan, as I replied before, I'm not a big fan of this approach. For
now, I think it would be best to add private wrapper in libsas to
support deferring unchained work items while draining.
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 02/24] workqueue: defer work to a draining queue
2011-12-19 21:49 ` Tejun Heo
@ 2011-12-19 22:01 ` Dan Williams
2011-12-19 22:08 ` Tejun Heo
0 siblings, 1 reply; 39+ messages in thread
From: Dan Williams @ 2011-12-19 22:01 UTC (permalink / raw)
To: Tejun Heo; +Cc: linux-scsi, linux-ide
On Mon, Dec 19, 2011 at 1:49 PM, Tejun Heo <tj@kernel.org> wrote:
> Hello,
>
> On Fri, Dec 16, 2011 at 06:33:18PM -0800, Dan Williams wrote:
>> commit 9c5a2ba7 "workqueue: separate out drain_workqueue() from
>> destroy_workqueue()" provided drain_workqueue() for users like libsas to
>> use for flushing events.
>>
>> When libsas drains it wants currently queued and chained events to be
>> flushed, but it fully expects to continue issuing unchained events with
>> the expectation that they are serviced sometime after the drain.
>>
>> For external users of drain_workqueue() arrange for unchained work to be
>> queued after the drain completes, if the caller cares if unchained work
>> was queued as a result of the drain it can check for a non-zero return
>> value. Deferred work is guaranteed to be at least queued when
>> drain_workqueue() returns, and visible to flush_workqueue() users as
>> well.
>>
>> Unfortunately this causes the promotion of workqueue_lock to hard-irq
>> safe and does not guarantee that work submitted via queue_work_on() runs
>> on the specified cpu if it gets deferred.
>>
>> Cc: Tejun Heo <tj@kernel.org>
>> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>
> Dan, as I replied before, I'm not a big fan of this approach.
Sorry I must have missed it, I can't seem to find a reply in the archives?
> For
> now, I think it would be best to add private wrapper in libsas to
> support deferring unchained work items while draining.
Ok, a form of this was nak'd by James before [1], but I can try again
with pushing this chained submission checking down into scsi.
--
Dan
[1]: http://marc.info/?l=linux-scsi&m=130655248916967&w=2
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 02/24] workqueue: defer work to a draining queue
2011-12-19 22:01 ` Dan Williams
@ 2011-12-19 22:08 ` Tejun Heo
2011-12-19 22:25 ` Williams, Dan J
0 siblings, 1 reply; 39+ messages in thread
From: Tejun Heo @ 2011-12-19 22:08 UTC (permalink / raw)
To: Dan Williams; +Cc: linux-scsi, linux-ide
Hello,
On Mon, Dec 19, 2011 at 02:01:47PM -0800, Dan Williams wrote:
> > Dan, as I replied before, I'm not a big fan of this approach.
>
> Sorry I must have missed it, I can't seem to find a reply in the archives?
Yeah, I can't find it either. I definitely remember writing it.
Hmmm... weird. Either I'm finally losing my mind or it didn't get out
for some reason. Sorry. :)
> > For
> > now, I think it would be best to add private wrapper in libsas to
> > support deferring unchained work items while draining.
>
> Ok, a form of this was nak'd by James before [1], but I can try again
> with pushing this chained submission checking down into scsi.
The issues I see with the proposed change is,
* There doesn't seem to be high demand for it.
* It isn't implemented the right way - it introduces unnecessary and
hidden ordering between chained work items being drained and newly
queued unchained ones. We can try to do it properly without
affecting new unchained work items but I'm not sure the added
complexity is justified given the first issue.
I don't think adding a wrapper which defers queueing while draining is
going on would be too complex, right?
Thank you.
--
tejun
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 02/24] workqueue: defer work to a draining queue
2011-12-19 22:08 ` Tejun Heo
@ 2011-12-19 22:25 ` Williams, Dan J
0 siblings, 0 replies; 39+ messages in thread
From: Williams, Dan J @ 2011-12-19 22:25 UTC (permalink / raw)
To: Tejun Heo; +Cc: linux-scsi, linux-ide
On Mon, Dec 19, 2011 at 2:08 PM, Tejun Heo <tj@kernel.org> wrote:
> Hello,
>
> On Mon, Dec 19, 2011 at 02:01:47PM -0800, Dan Williams wrote:
>> > Dan, as I replied before, I'm not a big fan of this approach.
>>
>> Sorry I must have missed it, I can't seem to find a reply in the archives?
>
> Yeah, I can't find it either. I definitely remember writing it.
> Hmmm... weird. Either I'm finally losing my mind or it didn't get out
> for some reason. Sorry. :)
As long as it's you losing your mind and not me :-).
>> > For
>> > now, I think it would be best to add private wrapper in libsas to
>> > support deferring unchained work items while draining.
>>
>> Ok, a form of this was nak'd by James before [1], but I can try again
>> with pushing this chained submission checking down into scsi.
>
> The issues I see with the proposed change is,
>
> * There doesn't seem to be high demand for it.
>
> * It isn't implemented the right way - it introduces unnecessary and
> hidden ordering between chained work items being drained and newly
> queued unchained ones. We can try to do it properly without
> affecting new unchained work items but I'm not sure the added
> complexity is justified given the first issue.
Fair enough.
> I don't think adding a wrapper which defers queueing while draining is
> going on would be too complex, right?
No, I think it should be pretty straightforward to teach libsas not to
throw anything else at the queue while a drain is pending.
Thanks,
Dan
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH 03/24] scsi: use drain_workqueue
2011-12-17 2:33 [PATCH 00/24] libsas: eh reworks (ata-eh vs discovery, races, ...) Dan Williams
2011-12-17 2:33 ` [PATCH 01/24] workqueue: promote workqueue_lock to hard-irq safe Dan Williams
2011-12-17 2:33 ` [PATCH 02/24] workqueue: defer work to a draining queue Dan Williams
@ 2011-12-17 2:33 ` Dan Williams
2011-12-17 2:33 ` [PATCH 04/24] libsas: remove unused ata_task_resp fields Dan Williams
` (20 subsequent siblings)
23 siblings, 0 replies; 39+ messages in thread
From: Dan Williams @ 2011-12-17 2:33 UTC (permalink / raw)
To: linux-scsi; +Cc: linux-ide, Mike Christie, Robert Love
Use 'drain' versus 'flush' as the former additionally flushes chained
operations. libsas uses chained operations when it posts discovery work
in response to a port event.
As a result the hardcoded double-flush can be removed from the isci
driver.
Cc: Mike Christie <michaelc@cs.wisc.edu>
Cc: Robert Love <robert.w.love@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/scsi/hosts.c | 8 ++++----
drivers/scsi/isci/host.c | 3 ---
include/scsi/scsi_host.h | 2 +-
3 files changed, 5 insertions(+), 8 deletions(-)
diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 351dc0b..37155d1 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -564,19 +564,19 @@ int scsi_queue_work(struct Scsi_Host *shost, struct work_struct *work)
EXPORT_SYMBOL_GPL(scsi_queue_work);
/**
- * scsi_flush_work - Flush a Scsi_Host's workqueue.
+ * scsi_flush_work - Drain a Scsi_Host's workqueue.
* @shost: Pointer to Scsi_Host.
**/
-void scsi_flush_work(struct Scsi_Host *shost)
+int scsi_flush_work(struct Scsi_Host *shost)
{
if (!shost->work_q) {
printk(KERN_ERR
"ERROR: Scsi host '%s' attempted to flush scsi-work, "
"when no workqueue created.\n", shost->hostt->name);
dump_stack();
- return;
+ return 0;
}
- flush_workqueue(shost->work_q);
+ return drain_workqueue(shost->work_q);
}
EXPORT_SYMBOL_GPL(scsi_flush_work);
diff --git a/drivers/scsi/isci/host.c b/drivers/scsi/isci/host.c
index e7fe9c4..240779a 100644
--- a/drivers/scsi/isci/host.c
+++ b/drivers/scsi/isci/host.c
@@ -655,9 +655,6 @@ int isci_host_scan_finished(struct Scsi_Host *shost, unsigned long time)
if (test_bit(IHOST_START_PENDING, &ihost->flags))
return 0;
- /* todo: use sas_flush_discovery once it is upstream */
- scsi_flush_work(shost);
-
scsi_flush_work(shost);
dev_dbg(&ihost->pdev->dev,
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 50266c9..505bc34 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -770,7 +770,7 @@ static inline int scsi_host_in_recovery(struct Scsi_Host *shost)
}
extern int scsi_queue_work(struct Scsi_Host *, struct work_struct *);
-extern void scsi_flush_work(struct Scsi_Host *);
+extern int scsi_flush_work(struct Scsi_Host *);
extern struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *, int);
extern int __must_check scsi_add_host_with_dma(struct Scsi_Host *,
^ permalink raw reply related [flat|nested] 39+ messages in thread* [PATCH 04/24] libsas: remove unused ata_task_resp fields
2011-12-17 2:33 [PATCH 00/24] libsas: eh reworks (ata-eh vs discovery, races, ...) Dan Williams
` (2 preceding siblings ...)
2011-12-17 2:33 ` [PATCH 03/24] scsi: use drain_workqueue Dan Williams
@ 2011-12-17 2:33 ` Dan Williams
2011-12-17 12:51 ` Sergei Shtylyov
2011-12-19 1:38 ` Jack Wang
2011-12-17 2:33 ` [PATCH 05/24] libsas: kill sas_slave_destroy Dan Williams
` (19 subsequent siblings)
23 siblings, 2 replies; 39+ messages in thread
From: Dan Williams @ 2011-12-17 2:33 UTC (permalink / raw)
To: linux-scsi; +Cc: linux-ide
Commit 1e34c838 removed the routines to fake the presence of the sata
control registers, now remove the unused data structure fields to kill
any remaining confusion.
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/scsi/libsas/sas_ata.c | 4 ----
include/scsi/libsas.h | 7 -------
2 files changed, 0 insertions(+), 11 deletions(-)
diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index db9238f..83118d0 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -121,10 +121,6 @@ static void sas_ata_task_done(struct sas_task *task)
if (unlikely(link->eh_info.err_mask))
qc->flags |= ATA_QCFLAG_FAILED;
}
-
- dev->sata_dev.sstatus = resp->sstatus;
- dev->sata_dev.serror = resp->serror;
- dev->sata_dev.scontrol = resp->scontrol;
} else {
ac = sas_to_ata_err(stat);
if (ac) {
diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index 6a308d4..6e64b03 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -171,9 +171,6 @@ struct sata_device {
struct ata_port *ap;
struct ata_host ata_host;
struct ata_taskfile tf;
- u32 sstatus;
- u32 serror;
- u32 scontrol;
};
/* ---------- Domain device ---------- */
@@ -487,10 +484,6 @@ enum exec_status {
struct ata_task_resp {
u16 frame_len;
u8 ending_fis[24]; /* dev to host or data-in */
- u32 sstatus;
- u32 serror;
- u32 scontrol;
- u32 sactive;
};
#define SAS_STATUS_BUF_SIZE 96
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH 04/24] libsas: remove unused ata_task_resp fields
2011-12-17 2:33 ` [PATCH 04/24] libsas: remove unused ata_task_resp fields Dan Williams
@ 2011-12-17 12:51 ` Sergei Shtylyov
2011-12-19 1:38 ` Jack Wang
1 sibling, 0 replies; 39+ messages in thread
From: Sergei Shtylyov @ 2011-12-17 12:51 UTC (permalink / raw)
To: Dan Williams; +Cc: linux-scsi, linux-ide
Hello.
On 17-12-2011 6:33, Dan Williams wrote:
> Commit 1e34c838 removed the routines to fake the presence of the sata
Please also specify that commit's summary (in parens).
> control registers, now remove the unused data structure fields to kill
> any remaining confusion.
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
MBR, Sergei
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH 04/24] libsas: remove unused ata_task_resp fields
2011-12-17 2:33 ` [PATCH 04/24] libsas: remove unused ata_task_resp fields Dan Williams
2011-12-17 12:51 ` Sergei Shtylyov
@ 2011-12-19 1:38 ` Jack Wang
1 sibling, 0 replies; 39+ messages in thread
From: Jack Wang @ 2011-12-19 1:38 UTC (permalink / raw)
To: 'Dan Williams', linux-scsi; +Cc: linux-ide
>
[Jack Wang]
Thanks for looking for this, looks OK for me.
Acked-by: Jack Wang <jack_wang@usish.com>
> Commit 1e34c838 removed the routines to fake the presence of the sata
> control registers, now remove the unused data structure fields to kill
> any remaining confusion.
>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
> drivers/scsi/libsas/sas_ata.c | 4 ----
> include/scsi/libsas.h | 7 -------
> 2 files changed, 0 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
> index db9238f..83118d0 100644
> --- a/drivers/scsi/libsas/sas_ata.c
> +++ b/drivers/scsi/libsas/sas_ata.c
> @@ -121,10 +121,6 @@ static void sas_ata_task_done(struct sas_task *task)
> if (unlikely(link->eh_info.err_mask))
> qc->flags |= ATA_QCFLAG_FAILED;
> }
> -
> - dev->sata_dev.sstatus = resp->sstatus;
> - dev->sata_dev.serror = resp->serror;
> - dev->sata_dev.scontrol = resp->scontrol;
> } else {
> ac = sas_to_ata_err(stat);
> if (ac) {
> diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
> index 6a308d4..6e64b03 100644
> --- a/include/scsi/libsas.h
> +++ b/include/scsi/libsas.h
> @@ -171,9 +171,6 @@ struct sata_device {
> struct ata_port *ap;
> struct ata_host ata_host;
> struct ata_taskfile tf;
> - u32 sstatus;
> - u32 serror;
> - u32 scontrol;
> };
>
> /* ---------- Domain device ---------- */
> @@ -487,10 +484,6 @@ enum exec_status {
> struct ata_task_resp {
> u16 frame_len;
> u8 ending_fis[24]; /* dev to host or data-in */
> - u32 sstatus;
> - u32 serror;
> - u32 scontrol;
> - u32 sactive;
> };
>
> #define SAS_STATUS_BUF_SIZE 96
>
> --
> 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] 39+ messages in thread
* [PATCH 05/24] libsas: kill sas_slave_destroy
2011-12-17 2:33 [PATCH 00/24] libsas: eh reworks (ata-eh vs discovery, races, ...) Dan Williams
` (3 preceding siblings ...)
2011-12-17 2:33 ` [PATCH 04/24] libsas: remove unused ata_task_resp fields Dan Williams
@ 2011-12-17 2:33 ` Dan Williams
2011-12-17 2:33 ` [PATCH 06/24] libsas: fix domain_device leak Dan Williams
` (18 subsequent siblings)
23 siblings, 0 replies; 39+ messages in thread
From: Dan Williams @ 2011-12-17 2:33 UTC (permalink / raw)
To: linux-scsi; +Cc: linux-ide
Per commit 3e4ec344 "libata: kill ATA_FLAG_DISABLED" needing to set
ATA_DEV_NONE is a holdover from before libsas converted to the
"new-style" ata-eh.
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/scsi/aic94xx/aic94xx_init.c | 1 -
drivers/scsi/isci/init.c | 1 -
drivers/scsi/libsas/sas_scsi_host.c | 9 ---------
drivers/scsi/mvsas/mv_init.c | 1 -
drivers/scsi/pm8001/pm8001_init.c | 1 -
include/scsi/libsas.h | 1 -
6 files changed, 0 insertions(+), 14 deletions(-)
diff --git a/drivers/scsi/aic94xx/aic94xx_init.c b/drivers/scsi/aic94xx/aic94xx_init.c
index d5ff142..8db4e72 100644
--- a/drivers/scsi/aic94xx/aic94xx_init.c
+++ b/drivers/scsi/aic94xx/aic94xx_init.c
@@ -68,7 +68,6 @@ static struct scsi_host_template aic94xx_sht = {
.queuecommand = sas_queuecommand,
.target_alloc = sas_target_alloc,
.slave_configure = sas_slave_configure,
- .slave_destroy = sas_slave_destroy,
.scan_finished = asd_scan_finished,
.scan_start = asd_scan_start,
.change_queue_depth = sas_change_queue_depth,
diff --git a/drivers/scsi/isci/init.c b/drivers/scsi/isci/init.c
index a97edab..f988c16 100644
--- a/drivers/scsi/isci/init.c
+++ b/drivers/scsi/isci/init.c
@@ -146,7 +146,6 @@ static struct scsi_host_template isci_sht = {
.queuecommand = sas_queuecommand,
.target_alloc = sas_target_alloc,
.slave_configure = sas_slave_configure,
- .slave_destroy = sas_slave_destroy,
.scan_finished = isci_host_scan_finished,
.scan_start = isci_host_scan_start,
.change_queue_depth = sas_change_queue_depth,
diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c
index b6e233d..e95e5e1 100644
--- a/drivers/scsi/libsas/sas_scsi_host.c
+++ b/drivers/scsi/libsas/sas_scsi_host.c
@@ -797,14 +797,6 @@ int sas_slave_configure(struct scsi_device *scsi_dev)
return 0;
}
-void sas_slave_destroy(struct scsi_device *scsi_dev)
-{
- struct domain_device *dev = sdev_to_domain_dev(scsi_dev);
-
- if (dev_is_sata(dev))
- sas_to_ata_dev(dev)->class = ATA_DEV_NONE;
-}
-
int sas_change_queue_depth(struct scsi_device *sdev, int depth, int reason)
{
struct domain_device *dev = sdev_to_domain_dev(sdev);
@@ -1108,7 +1100,6 @@ EXPORT_SYMBOL_GPL(sas_request_addr);
EXPORT_SYMBOL_GPL(sas_queuecommand);
EXPORT_SYMBOL_GPL(sas_target_alloc);
EXPORT_SYMBOL_GPL(sas_slave_configure);
-EXPORT_SYMBOL_GPL(sas_slave_destroy);
EXPORT_SYMBOL_GPL(sas_change_queue_depth);
EXPORT_SYMBOL_GPL(sas_change_queue_type);
EXPORT_SYMBOL_GPL(sas_bios_param);
diff --git a/drivers/scsi/mvsas/mv_init.c b/drivers/scsi/mvsas/mv_init.c
index 6f58919..d45878b 100644
--- a/drivers/scsi/mvsas/mv_init.c
+++ b/drivers/scsi/mvsas/mv_init.c
@@ -60,7 +60,6 @@ static struct scsi_host_template mvs_sht = {
.queuecommand = sas_queuecommand,
.target_alloc = sas_target_alloc,
.slave_configure = sas_slave_configure,
- .slave_destroy = sas_slave_destroy,
.scan_finished = mvs_scan_finished,
.scan_start = mvs_scan_start,
.change_queue_depth = sas_change_queue_depth,
diff --git a/drivers/scsi/pm8001/pm8001_init.c b/drivers/scsi/pm8001/pm8001_init.c
index c21a216..bd165ea 100644
--- a/drivers/scsi/pm8001/pm8001_init.c
+++ b/drivers/scsi/pm8001/pm8001_init.c
@@ -62,7 +62,6 @@ static struct scsi_host_template pm8001_sht = {
.queuecommand = sas_queuecommand,
.target_alloc = sas_target_alloc,
.slave_configure = sas_slave_configure,
- .slave_destroy = sas_slave_destroy,
.scan_finished = pm8001_scan_finished,
.scan_start = pm8001_scan_start,
.change_queue_depth = sas_change_queue_depth,
diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index 6e64b03..2b14348 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -625,7 +625,6 @@ extern int sas_queuecommand(struct Scsi_Host * ,struct scsi_cmnd *);
extern int sas_target_alloc(struct scsi_target *);
extern int sas_slave_alloc(struct scsi_device *);
extern int sas_slave_configure(struct scsi_device *);
-extern void sas_slave_destroy(struct scsi_device *);
extern int sas_change_queue_depth(struct scsi_device *, int new_depth,
int reason);
extern int sas_change_queue_type(struct scsi_device *, int qt);
^ permalink raw reply related [flat|nested] 39+ messages in thread* [PATCH 06/24] libsas: fix domain_device leak
2011-12-17 2:33 [PATCH 00/24] libsas: eh reworks (ata-eh vs discovery, races, ...) Dan Williams
` (4 preceding siblings ...)
2011-12-17 2:33 ` [PATCH 05/24] libsas: kill sas_slave_destroy Dan Williams
@ 2011-12-17 2:33 ` Dan Williams
2011-12-19 2:32 ` Jack Wang
2011-12-17 2:33 ` [PATCH 07/24] libsas: fix leak of dev->sata_dev.identify_[packet_]device Dan Williams
` (17 subsequent siblings)
23 siblings, 1 reply; 39+ messages in thread
From: Dan Williams @ 2011-12-17 2:33 UTC (permalink / raw)
To: linux-scsi; +Cc: linux-ide
Arrange for the deallocation of a struct domain_device object when it no
longer has:
1/ any children
2/ references by any scsi_targets
3/ references by a lldd
The comment about domain_device lifetime in
Documentation/scsi/libsas.txt is stale as it appears mainline never had
a version of a struct domain_device that was registered as a kobject.
We now manage domain_device reference counts on behalf of external
agents.
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
Documentation/scsi/libsas.txt | 15 ---------------
drivers/scsi/libsas/sas_discover.c | 36 +++++++++++++++++++++++------------
drivers/scsi/libsas/sas_expander.c | 10 ++++++----
drivers/scsi/libsas/sas_internal.h | 19 ++++++++++++++++++
drivers/scsi/libsas/sas_scsi_host.c | 6 ++++--
include/scsi/libsas.h | 1 +
6 files changed, 54 insertions(+), 33 deletions(-)
diff --git a/Documentation/scsi/libsas.txt b/Documentation/scsi/libsas.txt
index aa54f54..3cc9c78 100644
--- a/Documentation/scsi/libsas.txt
+++ b/Documentation/scsi/libsas.txt
@@ -398,21 +398,6 @@ struct sas_task {
task_done -- callback when the task has finished execution
};
-When an external entity, entity other than the LLDD or the
-SAS Layer, wants to work with a struct domain_device, it
-_must_ call kobject_get() when getting a handle on the
-device and kobject_put() when it is done with the device.
-
-This does two things:
- A) implements proper kfree() for the device;
- B) increments/decrements the kref for all players:
- domain_device
- all domain_device's ... (if past an expander)
- port
- host adapter
- pci device
- and up the ladder, etc.
-
DISCOVERY
---------
diff --git a/drivers/scsi/libsas/sas_discover.c b/drivers/scsi/libsas/sas_discover.c
index 54a5199..4e64930 100644
--- a/drivers/scsi/libsas/sas_discover.c
+++ b/drivers/scsi/libsas/sas_discover.c
@@ -36,8 +36,6 @@
void sas_init_dev(struct domain_device *dev)
{
- INIT_LIST_HEAD(&dev->siblings);
- INIT_LIST_HEAD(&dev->dev_list_node);
switch (dev->dev_type) {
case SAS_END_DEV:
break;
@@ -73,14 +71,14 @@ static int sas_get_port_device(struct asd_sas_port *port)
struct sas_rphy *rphy;
struct domain_device *dev;
- dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+ dev = sas_alloc_device();
if (!dev)
return -ENOMEM;
spin_lock_irqsave(&port->phy_list_lock, flags);
if (list_empty(&port->phy_list)) {
spin_unlock_irqrestore(&port->phy_list_lock, flags);
- kfree(dev);
+ sas_put_device(dev);
return -ENODEV;
}
phy = container_of(port->phy_list.next, struct asd_sas_phy, port_phy_el);
@@ -130,7 +128,7 @@ static int sas_get_port_device(struct asd_sas_port *port)
}
if (!rphy) {
- kfree(dev);
+ sas_put_device(dev);
return -ENODEV;
}
rphy->identify.phy_identifier = phy->phy->identify.phy_identifier;
@@ -173,6 +171,7 @@ int sas_notify_lldd_dev_found(struct domain_device *dev)
dev_name(sas_ha->dev),
SAS_ADDR(dev->sas_addr), res);
}
+ kref_get(&dev->kref);
}
return res;
}
@@ -184,8 +183,10 @@ void sas_notify_lldd_dev_gone(struct domain_device *dev)
struct Scsi_Host *shost = sas_ha->core.shost;
struct sas_internal *i = to_sas_internal(shost->transportt);
- if (i->dft->lldd_dev_gone)
+ if (i->dft->lldd_dev_gone) {
i->dft->lldd_dev_gone(dev);
+ sas_put_device(dev);
+ }
}
/* ---------- Common/dispatchers ---------- */
@@ -219,6 +220,20 @@ out_err2:
/* ---------- Device registration and unregistration ---------- */
+void sas_free_device(struct kref *kref)
+{
+ struct domain_device *dev = container_of(kref, typeof(*dev), kref);
+
+ if (dev->parent)
+ sas_put_device(dev->parent);
+
+ /* remove the phys and ports, everything else should be gone */
+ if (dev->dev_type == EDGE_DEV || dev->dev_type == FANOUT_DEV)
+ kfree(dev->ex_dev.ex_phy);
+
+ kfree(dev);
+}
+
static void sas_unregister_common_dev(struct asd_sas_port *port, struct domain_device *dev)
{
sas_notify_lldd_dev_gone(dev);
@@ -230,6 +245,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);
spin_unlock_irq(&port->dev_list_lock);
+
+ sas_put_device(dev);
}
void sas_unregister_dev(struct asd_sas_port *port, struct domain_device *dev)
@@ -239,11 +256,6 @@ void sas_unregister_dev(struct asd_sas_port *port, struct domain_device *dev)
sas_rphy_delete(dev->rphy);
dev->rphy = NULL;
}
- if (dev->dev_type == EDGE_DEV || dev->dev_type == FANOUT_DEV) {
- /* remove the phys and ports, everything else should be gone */
- kfree(dev->ex_dev.ex_phy);
- dev->ex_dev.ex_phy = NULL;
- }
sas_unregister_common_dev(port, dev);
}
@@ -322,7 +334,7 @@ static void sas_discover_domain(struct work_struct *work)
list_del_init(&dev->dev_list_node);
spin_unlock_irq(&port->dev_list_lock);
- kfree(dev); /* not kobject_register-ed yet */
+ sas_put_device(dev);
port->port_dev = NULL;
}
diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index 1b831c5..15d2239 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -657,10 +657,11 @@ static struct domain_device *sas_ex_discover_end_dev(
if (phy->attached_sata_host || phy->attached_sata_ps)
return NULL;
- child = kzalloc(sizeof(*child), GFP_KERNEL);
+ child = sas_alloc_device();
if (!child)
return NULL;
+ kref_get(&parent->kref);
child->parent = parent;
child->port = parent->port;
child->iproto = phy->attached_iproto;
@@ -762,7 +763,7 @@ static struct domain_device *sas_ex_discover_end_dev(
sas_port_delete(phy->port);
out_err:
phy->port = NULL;
- kfree(child);
+ sas_put_device(child);
return NULL;
}
@@ -809,7 +810,7 @@ static struct domain_device *sas_ex_discover_expander(
phy->attached_phy_id);
return NULL;
}
- child = kzalloc(sizeof(*child), GFP_KERNEL);
+ child = sas_alloc_device();
if (!child)
return NULL;
@@ -835,6 +836,7 @@ static struct domain_device *sas_ex_discover_expander(
child->rphy = rphy;
edev = rphy_to_expander_device(rphy);
child->dev_type = phy->attached_dev_type;
+ kref_get(&parent->kref);
child->parent = parent;
child->port = port;
child->iproto = phy->attached_iproto;
@@ -858,7 +860,7 @@ static struct domain_device *sas_ex_discover_expander(
spin_lock_irq(&parent->port->dev_list_lock);
list_del(&child->dev_list_node);
spin_unlock_irq(&parent->port->dev_list_lock);
- kfree(child);
+ sas_put_device(child);
return NULL;
}
list_add_tail(&child->siblings, &parent->ex_dev.children);
diff --git a/drivers/scsi/libsas/sas_internal.h b/drivers/scsi/libsas/sas_internal.h
index 14e21b5..0d43408 100644
--- a/drivers/scsi/libsas/sas_internal.h
+++ b/drivers/scsi/libsas/sas_internal.h
@@ -76,6 +76,8 @@ struct domain_device *sas_find_dev_by_rphy(struct sas_rphy *rphy);
void sas_hae_reset(struct work_struct *work);
+void sas_free_device(struct kref *kref);
+
#ifdef CONFIG_SCSI_SAS_HOST_SMP
extern int sas_smp_host_handler(struct Scsi_Host *shost, struct request *req,
struct request *rsp);
@@ -161,4 +163,21 @@ static inline void sas_add_parent_port(struct domain_device *dev, int phy_id)
sas_port_add_phy(ex->parent_port, ex_phy->phy);
}
+static inline struct domain_device *sas_alloc_device(void)
+{
+ struct domain_device *dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+
+ if (dev) {
+ INIT_LIST_HEAD(&dev->siblings);
+ INIT_LIST_HEAD(&dev->dev_list_node);
+ kref_init(&dev->kref);
+ }
+ return dev;
+}
+
+static inline void sas_put_device(struct domain_device *dev)
+{
+ kref_put(&dev->kref, sas_free_device);
+}
+
#endif /* _SAS_INTERNAL_H_ */
diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c
index e95e5e1..4636453 100644
--- a/drivers/scsi/libsas/sas_scsi_host.c
+++ b/drivers/scsi/libsas/sas_scsi_host.c
@@ -758,6 +758,7 @@ int sas_target_alloc(struct scsi_target *starget)
return res;
}
+ kref_get(&found_dev->kref);
starget->hostdata = found_dev;
return 0;
}
@@ -1047,7 +1048,7 @@ int sas_slave_alloc(struct scsi_device *scsi_dev)
void sas_target_destroy(struct scsi_target *starget)
{
- struct domain_device *found_dev = sas_find_target(starget);
+ struct domain_device *found_dev = starget->hostdata;
if (!found_dev)
return;
@@ -1055,7 +1056,8 @@ void sas_target_destroy(struct scsi_target *starget)
if (dev_is_sata(found_dev))
ata_sas_port_destroy(found_dev->sata_dev.ap);
- return;
+ starget->hostdata = NULL;
+ sas_put_device(found_dev);
}
static void sas_parse_addr(u8 *sas_addr, const char *p)
diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index 2b14348..7ecb5c1 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -206,6 +206,7 @@ struct domain_device {
void *lldd_dev;
int gone;
+ struct kref kref;
};
struct sas_discovery_event {
^ permalink raw reply related [flat|nested] 39+ messages in thread* RE: [PATCH 06/24] libsas: fix domain_device leak
2011-12-17 2:33 ` [PATCH 06/24] libsas: fix domain_device leak Dan Williams
@ 2011-12-19 2:32 ` Jack Wang
0 siblings, 0 replies; 39+ messages in thread
From: Jack Wang @ 2011-12-19 2:32 UTC (permalink / raw)
To: 'Dan Williams', linux-scsi; +Cc: linux-ide
Hi, Dan,
Thanks for fix.
Reviewed-by: Jack Wang <jack_wang@usish.com>
: [PATCH 06/24] libsas: fix domain_device leak
>
> Arrange for the deallocation of a struct domain_device object when it no
> longer has:
> 1/ any children
> 2/ references by any scsi_targets
> 3/ references by a lldd
>
> The comment about domain_device lifetime in
> Documentation/scsi/libsas.txt is stale as it appears mainline never had
> a version of a struct domain_device that was registered as a kobject.
> We now manage domain_device reference counts on behalf of external
> agents.
>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
> Documentation/scsi/libsas.txt | 15 ---------------
> drivers/scsi/libsas/sas_discover.c | 36
> +++++++++++++++++++++++------------
> drivers/scsi/libsas/sas_expander.c | 10 ++++++----
> drivers/scsi/libsas/sas_internal.h | 19 ++++++++++++++++++
> drivers/scsi/libsas/sas_scsi_host.c | 6 ++++--
> include/scsi/libsas.h | 1 +
> 6 files changed, 54 insertions(+), 33 deletions(-)
>
> diff --git a/Documentation/scsi/libsas.txt b/Documentation/scsi/libsas.txt
> index aa54f54..3cc9c78 100644
> --- a/Documentation/scsi/libsas.txt
> +++ b/Documentation/scsi/libsas.txt
> @@ -398,21 +398,6 @@ struct sas_task {
> task_done -- callback when the task has finished execution
> };
>
> -When an external entity, entity other than the LLDD or the
> -SAS Layer, wants to work with a struct domain_device, it
> -_must_ call kobject_get() when getting a handle on the
> -device and kobject_put() when it is done with the device.
> -
> -This does two things:
> - A) implements proper kfree() for the device;
> - B) increments/decrements the kref for all players:
> - domain_device
> - all domain_device's ... (if past an expander)
> - port
> - host adapter
> - pci device
> - and up the ladder, etc.
> -
> DISCOVERY
> ---------
>
> diff --git a/drivers/scsi/libsas/sas_discover.c
> b/drivers/scsi/libsas/sas_discover.c
> index 54a5199..4e64930 100644
> --- a/drivers/scsi/libsas/sas_discover.c
> +++ b/drivers/scsi/libsas/sas_discover.c
> @@ -36,8 +36,6 @@
>
> void sas_init_dev(struct domain_device *dev)
> {
> - INIT_LIST_HEAD(&dev->siblings);
> - INIT_LIST_HEAD(&dev->dev_list_node);
> switch (dev->dev_type) {
> case SAS_END_DEV:
> break;
> @@ -73,14 +71,14 @@ static int sas_get_port_device(struct asd_sas_port
*port)
> struct sas_rphy *rphy;
> struct domain_device *dev;
>
> - dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> + dev = sas_alloc_device();
> if (!dev)
> return -ENOMEM;
>
> spin_lock_irqsave(&port->phy_list_lock, flags);
> if (list_empty(&port->phy_list)) {
> spin_unlock_irqrestore(&port->phy_list_lock, flags);
> - kfree(dev);
> + sas_put_device(dev);
> return -ENODEV;
> }
> phy = container_of(port->phy_list.next, struct asd_sas_phy,
> port_phy_el);
> @@ -130,7 +128,7 @@ static int sas_get_port_device(struct asd_sas_port
*port)
> }
>
> if (!rphy) {
> - kfree(dev);
> + sas_put_device(dev);
> return -ENODEV;
> }
> rphy->identify.phy_identifier = phy->phy->identify.phy_identifier;
> @@ -173,6 +171,7 @@ int sas_notify_lldd_dev_found(struct domain_device
*dev)
> dev_name(sas_ha->dev),
> SAS_ADDR(dev->sas_addr), res);
> }
> + kref_get(&dev->kref);
> }
> return res;
> }
> @@ -184,8 +183,10 @@ void sas_notify_lldd_dev_gone(struct domain_device
*dev)
> struct Scsi_Host *shost = sas_ha->core.shost;
> struct sas_internal *i = to_sas_internal(shost->transportt);
>
> - if (i->dft->lldd_dev_gone)
> + if (i->dft->lldd_dev_gone) {
> i->dft->lldd_dev_gone(dev);
> + sas_put_device(dev);
> + }
> }
>
> /* ---------- Common/dispatchers ---------- */
> @@ -219,6 +220,20 @@ out_err2:
>
> /* ---------- Device registration and unregistration ---------- */
>
> +void sas_free_device(struct kref *kref)
> +{
> + struct domain_device *dev = container_of(kref, typeof(*dev), kref);
> +
> + if (dev->parent)
> + sas_put_device(dev->parent);
> +
> + /* remove the phys and ports, everything else should be gone */
> + if (dev->dev_type == EDGE_DEV || dev->dev_type == FANOUT_DEV)
> + kfree(dev->ex_dev.ex_phy);
> +
> + kfree(dev);
> +}
> +
> static void sas_unregister_common_dev(struct asd_sas_port *port, struct
> domain_device *dev)
> {
> sas_notify_lldd_dev_gone(dev);
> @@ -230,6 +245,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);
> spin_unlock_irq(&port->dev_list_lock);
> +
> + sas_put_device(dev);
> }
>
> void sas_unregister_dev(struct asd_sas_port *port, struct domain_device
> *dev)
> @@ -239,11 +256,6 @@ void sas_unregister_dev(struct asd_sas_port *port,
struct
> domain_device *dev)
> sas_rphy_delete(dev->rphy);
> dev->rphy = NULL;
> }
> - if (dev->dev_type == EDGE_DEV || dev->dev_type == FANOUT_DEV) {
> - /* remove the phys and ports, everything else should be gone
*/
> - kfree(dev->ex_dev.ex_phy);
> - dev->ex_dev.ex_phy = NULL;
> - }
> sas_unregister_common_dev(port, dev);
> }
>
> @@ -322,7 +334,7 @@ static void sas_discover_domain(struct work_struct
*work)
> list_del_init(&dev->dev_list_node);
> spin_unlock_irq(&port->dev_list_lock);
>
> - kfree(dev); /* not kobject_register-ed yet */
> + sas_put_device(dev);
> port->port_dev = NULL;
> }
>
> diff --git a/drivers/scsi/libsas/sas_expander.c
> b/drivers/scsi/libsas/sas_expander.c
> index 1b831c5..15d2239 100644
> --- a/drivers/scsi/libsas/sas_expander.c
> +++ b/drivers/scsi/libsas/sas_expander.c
> @@ -657,10 +657,11 @@ static struct domain_device
*sas_ex_discover_end_dev(
> if (phy->attached_sata_host || phy->attached_sata_ps)
> return NULL;
>
> - child = kzalloc(sizeof(*child), GFP_KERNEL);
> + child = sas_alloc_device();
> if (!child)
> return NULL;
>
> + kref_get(&parent->kref);
> child->parent = parent;
> child->port = parent->port;
> child->iproto = phy->attached_iproto;
> @@ -762,7 +763,7 @@ static struct domain_device *sas_ex_discover_end_dev(
> sas_port_delete(phy->port);
> out_err:
> phy->port = NULL;
> - kfree(child);
> + sas_put_device(child);
> return NULL;
> }
>
> @@ -809,7 +810,7 @@ static struct domain_device *sas_ex_discover_expander(
> phy->attached_phy_id);
> return NULL;
> }
> - child = kzalloc(sizeof(*child), GFP_KERNEL);
> + child = sas_alloc_device();
> if (!child)
> return NULL;
>
> @@ -835,6 +836,7 @@ static struct domain_device *sas_ex_discover_expander(
> child->rphy = rphy;
> edev = rphy_to_expander_device(rphy);
> child->dev_type = phy->attached_dev_type;
> + kref_get(&parent->kref);
> child->parent = parent;
> child->port = port;
> child->iproto = phy->attached_iproto;
> @@ -858,7 +860,7 @@ static struct domain_device *sas_ex_discover_expander(
> spin_lock_irq(&parent->port->dev_list_lock);
> list_del(&child->dev_list_node);
> spin_unlock_irq(&parent->port->dev_list_lock);
> - kfree(child);
> + sas_put_device(child);
> return NULL;
> }
> list_add_tail(&child->siblings, &parent->ex_dev.children);
> diff --git a/drivers/scsi/libsas/sas_internal.h
> b/drivers/scsi/libsas/sas_internal.h
> index 14e21b5..0d43408 100644
> --- a/drivers/scsi/libsas/sas_internal.h
> +++ b/drivers/scsi/libsas/sas_internal.h
> @@ -76,6 +76,8 @@ struct domain_device *sas_find_dev_by_rphy(struct
sas_rphy
> *rphy);
>
> void sas_hae_reset(struct work_struct *work);
>
> +void sas_free_device(struct kref *kref);
> +
> #ifdef CONFIG_SCSI_SAS_HOST_SMP
> extern int sas_smp_host_handler(struct Scsi_Host *shost, struct request
> *req,
> struct request *rsp);
> @@ -161,4 +163,21 @@ static inline void sas_add_parent_port(struct
> domain_device *dev, int phy_id)
> sas_port_add_phy(ex->parent_port, ex_phy->phy);
> }
>
> +static inline struct domain_device *sas_alloc_device(void)
> +{
> + struct domain_device *dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> +
> + if (dev) {
> + INIT_LIST_HEAD(&dev->siblings);
> + INIT_LIST_HEAD(&dev->dev_list_node);
> + kref_init(&dev->kref);
> + }
> + return dev;
> +}
> +
> +static inline void sas_put_device(struct domain_device *dev)
> +{
> + kref_put(&dev->kref, sas_free_device);
> +}
> +
> #endif /* _SAS_INTERNAL_H_ */
> diff --git a/drivers/scsi/libsas/sas_scsi_host.c
> b/drivers/scsi/libsas/sas_scsi_host.c
> index e95e5e1..4636453 100644
> --- a/drivers/scsi/libsas/sas_scsi_host.c
> +++ b/drivers/scsi/libsas/sas_scsi_host.c
> @@ -758,6 +758,7 @@ int sas_target_alloc(struct scsi_target *starget)
> return res;
> }
>
> + kref_get(&found_dev->kref);
> starget->hostdata = found_dev;
> return 0;
> }
> @@ -1047,7 +1048,7 @@ int sas_slave_alloc(struct scsi_device *scsi_dev)
>
> void sas_target_destroy(struct scsi_target *starget)
> {
> - struct domain_device *found_dev = sas_find_target(starget);
> + struct domain_device *found_dev = starget->hostdata;
>
> if (!found_dev)
> return;
> @@ -1055,7 +1056,8 @@ void sas_target_destroy(struct scsi_target *starget)
> if (dev_is_sata(found_dev))
> ata_sas_port_destroy(found_dev->sata_dev.ap);
>
> - return;
> + starget->hostdata = NULL;
> + sas_put_device(found_dev);
> }
>
> static void sas_parse_addr(u8 *sas_addr, const char *p)
> diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
> index 2b14348..7ecb5c1 100644
> --- a/include/scsi/libsas.h
> +++ b/include/scsi/libsas.h
> @@ -206,6 +206,7 @@ struct domain_device {
>
> void *lldd_dev;
> int gone;
> + struct kref kref;
> };
>
> struct sas_discovery_event {
>
> --
> 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] 39+ messages in thread
* [PATCH 07/24] libsas: fix leak of dev->sata_dev.identify_[packet_]device
2011-12-17 2:33 [PATCH 00/24] libsas: eh reworks (ata-eh vs discovery, races, ...) Dan Williams
` (5 preceding siblings ...)
2011-12-17 2:33 ` [PATCH 06/24] libsas: fix domain_device leak Dan Williams
@ 2011-12-17 2:33 ` Dan Williams
2011-12-19 2:38 ` Jack Wang
2011-12-17 2:33 ` [PATCH 08/24] libsas: replace event locks with atomic bitops Dan Williams
` (16 subsequent siblings)
23 siblings, 1 reply; 39+ messages in thread
From: Dan Williams @ 2011-12-17 2:33 UTC (permalink / raw)
To: linux-scsi; +Cc: linux-ide
These are never freed in the nominal path. A domain_device has a
different lifetime than a sas_rphy we need a dev->rphy independent way
of identifying sata devices.
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/scsi/libsas/sas_discover.c | 6 ++++++
include/scsi/sas_ata.h | 3 ++-
2 files changed, 8 insertions(+), 1 deletions(-)
diff --git a/drivers/scsi/libsas/sas_discover.c b/drivers/scsi/libsas/sas_discover.c
index 4e64930..dc52b1f 100644
--- a/drivers/scsi/libsas/sas_discover.c
+++ b/drivers/scsi/libsas/sas_discover.c
@@ -30,6 +30,7 @@
#include <scsi/scsi_transport.h>
#include <scsi/scsi_transport_sas.h>
+#include <scsi/sas_ata.h>
#include "../scsi_sas_internal.h"
/* ---------- Basic task processing for discovery purposes ---------- */
@@ -231,6 +232,11 @@ void sas_free_device(struct kref *kref)
if (dev->dev_type == EDGE_DEV || dev->dev_type == FANOUT_DEV)
kfree(dev->ex_dev.ex_phy);
+ if (dev_is_sata(dev)) {
+ kfree(dev->sata_dev.identify_device);
+ kfree(dev->sata_dev.identify_packet_device);
+ }
+
kfree(dev);
}
diff --git a/include/scsi/sas_ata.h b/include/scsi/sas_ata.h
index 9c159f7..7d5013f 100644
--- a/include/scsi/sas_ata.h
+++ b/include/scsi/sas_ata.h
@@ -32,7 +32,8 @@
static inline int dev_is_sata(struct domain_device *dev)
{
- return (dev->rphy->identify.target_port_protocols & SAS_PROTOCOL_SATA);
+ return dev->dev_type == SATA_DEV || dev->dev_type == SATA_PM ||
+ dev->dev_type == SATA_PM_PORT;
}
int sas_ata_init_host_and_port(struct domain_device *found_dev,
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH 07/24] libsas: fix leak of dev->sata_dev.identify_[packet_]device
2011-12-17 2:33 ` [PATCH 07/24] libsas: fix leak of dev->sata_dev.identify_[packet_]device Dan Williams
@ 2011-12-19 2:38 ` Jack Wang
0 siblings, 0 replies; 39+ messages in thread
From: Jack Wang @ 2011-12-19 2:38 UTC (permalink / raw)
To: 'Dan Williams', linux-scsi; +Cc: linux-ide
Thanks for fix.
Reviewed-by: Jack Wang <jack_wang@usish.com>
>
> These are never freed in the nominal path. A domain_device has a
> different lifetime than a sas_rphy we need a dev->rphy independent way
> of identifying sata devices.
>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
> drivers/scsi/libsas/sas_discover.c | 6 ++++++
> include/scsi/sas_ata.h | 3 ++-
> 2 files changed, 8 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/scsi/libsas/sas_discover.c
> b/drivers/scsi/libsas/sas_discover.c
> index 4e64930..dc52b1f 100644
> --- a/drivers/scsi/libsas/sas_discover.c
> +++ b/drivers/scsi/libsas/sas_discover.c
> @@ -30,6 +30,7 @@
>
> #include <scsi/scsi_transport.h>
> #include <scsi/scsi_transport_sas.h>
> +#include <scsi/sas_ata.h>
> #include "../scsi_sas_internal.h"
>
> /* ---------- Basic task processing for discovery purposes ---------- */
> @@ -231,6 +232,11 @@ void sas_free_device(struct kref *kref)
> if (dev->dev_type == EDGE_DEV || dev->dev_type == FANOUT_DEV)
> kfree(dev->ex_dev.ex_phy);
>
> + if (dev_is_sata(dev)) {
> + kfree(dev->sata_dev.identify_device);
> + kfree(dev->sata_dev.identify_packet_device);
> + }
> +
> kfree(dev);
> }
>
> diff --git a/include/scsi/sas_ata.h b/include/scsi/sas_ata.h
> index 9c159f7..7d5013f 100644
> --- a/include/scsi/sas_ata.h
> +++ b/include/scsi/sas_ata.h
> @@ -32,7 +32,8 @@
>
> static inline int dev_is_sata(struct domain_device *dev)
> {
> - return (dev->rphy->identify.target_port_protocols &
> SAS_PROTOCOL_SATA);
> + return dev->dev_type == SATA_DEV || dev->dev_type == SATA_PM ||
> + dev->dev_type == SATA_PM_PORT;
> }
>
> int sas_ata_init_host_and_port(struct domain_device *found_dev,
>
> --
> 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] 39+ messages in thread
* [PATCH 08/24] libsas: replace event locks with atomic bitops
2011-12-17 2:33 [PATCH 00/24] libsas: eh reworks (ata-eh vs discovery, races, ...) Dan Williams
` (6 preceding siblings ...)
2011-12-17 2:33 ` [PATCH 07/24] libsas: fix leak of dev->sata_dev.identify_[packet_]device Dan Williams
@ 2011-12-17 2:33 ` Dan Williams
2011-12-17 2:33 ` [PATCH 09/24] libsas: remove ata_port.lock management duties from lldds Dan Williams
` (15 subsequent siblings)
23 siblings, 0 replies; 39+ messages in thread
From: Dan Williams @ 2011-12-17 2:33 UTC (permalink / raw)
To: linux-scsi; +Cc: linux-ide
The locks only served to make sure the pending event bitmask was updated
consistently.
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/scsi/libsas/sas_discover.c | 10 +++-------
drivers/scsi/libsas/sas_event.c | 8 +++-----
drivers/scsi/libsas/sas_init.c | 3 +--
drivers/scsi/libsas/sas_internal.h | 32 +++++++-------------------------
drivers/scsi/libsas/sas_phy.c | 12 ++++--------
drivers/scsi/libsas/sas_port.c | 15 +++++----------
include/scsi/libsas.h | 3 ---
7 files changed, 23 insertions(+), 60 deletions(-)
diff --git a/drivers/scsi/libsas/sas_discover.c b/drivers/scsi/libsas/sas_discover.c
index dc52b1f..ed04118 100644
--- a/drivers/scsi/libsas/sas_discover.c
+++ b/drivers/scsi/libsas/sas_discover.c
@@ -295,8 +295,7 @@ static void sas_discover_domain(struct work_struct *work)
container_of(work, struct sas_discovery_event, work);
struct asd_sas_port *port = ev->port;
- sas_begin_event(DISCE_DISCOVER_DOMAIN, &port->disc.disc_event_lock,
- &port->disc.pending);
+ clear_bit(DISCE_DISCOVER_DOMAIN, &port->disc.pending);
if (port->port_dev)
return;
@@ -355,8 +354,7 @@ static void sas_revalidate_domain(struct work_struct *work)
container_of(work, struct sas_discovery_event, work);
struct asd_sas_port *port = ev->port;
- sas_begin_event(DISCE_REVALIDATE_DOMAIN, &port->disc.disc_event_lock,
- &port->disc.pending);
+ clear_bit(DISCE_REVALIDATE_DOMAIN, &port->disc.pending);
SAS_DPRINTK("REVALIDATING DOMAIN on port %d, pid:%d\n", port->id,
task_pid_nr(current));
@@ -379,8 +377,7 @@ int sas_discover_event(struct asd_sas_port *port, enum discover_event ev)
BUG_ON(ev >= DISC_NUM_EVENTS);
- sas_queue_event(ev, &disc->disc_event_lock, &disc->pending,
- &disc->disc_work[ev].work, port->ha);
+ sas_queue_event(ev, &disc->pending, &disc->disc_work[ev].work, port->ha);
return 0;
}
@@ -400,7 +397,6 @@ void sas_init_disc(struct sas_discovery *disc, struct asd_sas_port *port)
[DISCE_REVALIDATE_DOMAIN] = sas_revalidate_domain,
};
- spin_lock_init(&disc->disc_event_lock);
disc->pending = 0;
for (i = 0; i < DISC_NUM_EVENTS; i++) {
INIT_WORK(&disc->disc_work[i].work, sas_event_fns[i]);
diff --git a/drivers/scsi/libsas/sas_event.c b/drivers/scsi/libsas/sas_event.c
index 9db30fb..9c084bc 100644
--- a/drivers/scsi/libsas/sas_event.c
+++ b/drivers/scsi/libsas/sas_event.c
@@ -30,7 +30,7 @@ static void notify_ha_event(struct sas_ha_struct *sas_ha, enum ha_event event)
{
BUG_ON(event >= HA_NUM_EVENTS);
- sas_queue_event(event, &sas_ha->event_lock, &sas_ha->pending,
+ sas_queue_event(event, &sas_ha->pending,
&sas_ha->ha_events[event].work, sas_ha);
}
@@ -40,7 +40,7 @@ static void notify_port_event(struct asd_sas_phy *phy, enum port_event event)
BUG_ON(event >= PORT_NUM_EVENTS);
- sas_queue_event(event, &ha->event_lock, &phy->port_events_pending,
+ sas_queue_event(event, &phy->port_events_pending,
&phy->port_events[event].work, ha);
}
@@ -50,7 +50,7 @@ static void notify_phy_event(struct asd_sas_phy *phy, enum phy_event event)
BUG_ON(event >= PHY_NUM_EVENTS);
- sas_queue_event(event, &ha->event_lock, &phy->phy_events_pending,
+ sas_queue_event(event, &phy->phy_events_pending,
&phy->phy_events[event].work, ha);
}
@@ -62,8 +62,6 @@ int sas_init_events(struct sas_ha_struct *sas_ha)
int i;
- spin_lock_init(&sas_ha->event_lock);
-
for (i = 0; i < HA_NUM_EVENTS; i++) {
INIT_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 d81c3b1..a435876 100644
--- a/drivers/scsi/libsas/sas_init.c
+++ b/drivers/scsi/libsas/sas_init.c
@@ -97,8 +97,7 @@ void sas_hae_reset(struct work_struct *work)
container_of(work, struct sas_ha_event, work);
struct sas_ha_struct *ha = ev->ha;
- sas_begin_event(HAE_RESET, &ha->event_lock,
- &ha->pending);
+ clear_bit(HAE_RESET, &ha->pending);
}
int sas_register_ha(struct sas_ha_struct *sas_ha)
diff --git a/drivers/scsi/libsas/sas_internal.h b/drivers/scsi/libsas/sas_internal.h
index 0d43408..7fe4ede 100644
--- a/drivers/scsi/libsas/sas_internal.h
+++ b/drivers/scsi/libsas/sas_internal.h
@@ -92,36 +92,18 @@ static inline int sas_smp_host_handler(struct Scsi_Host *shost,
}
#endif
-static inline void sas_queue_event(int event, spinlock_t *lock,
- unsigned long *pending,
+static inline void sas_queue_event(int event, unsigned long *pending,
struct work_struct *work,
struct sas_ha_struct *sas_ha)
{
- unsigned long flags;
+ if (!test_and_set_bit(event, pending)) {
+ unsigned long flags;
- spin_lock_irqsave(lock, flags);
- if (test_bit(event, pending)) {
- spin_unlock_irqrestore(lock, flags);
- return;
+ spin_lock_irqsave(&sas_ha->state_lock, flags);
+ if (sas_ha->state != SAS_HA_UNREGISTERED)
+ scsi_queue_work(sas_ha->core.shost, work);
+ spin_unlock_irqrestore(&sas_ha->state_lock, flags);
}
- __set_bit(event, pending);
- spin_unlock_irqrestore(lock, flags);
-
- spin_lock_irqsave(&sas_ha->state_lock, flags);
- if (sas_ha->state != SAS_HA_UNREGISTERED) {
- scsi_queue_work(sas_ha->core.shost, work);
- }
- spin_unlock_irqrestore(&sas_ha->state_lock, flags);
-}
-
-static inline void sas_begin_event(int event, spinlock_t *lock,
- unsigned long *pending)
-{
- unsigned long flags;
-
- spin_lock_irqsave(lock, flags);
- __clear_bit(event, pending);
- spin_unlock_irqrestore(lock, flags);
}
static inline void sas_fill_in_rphy(struct domain_device *dev,
diff --git a/drivers/scsi/libsas/sas_phy.c b/drivers/scsi/libsas/sas_phy.c
index e0f5018..dcfd4a9 100644
--- a/drivers/scsi/libsas/sas_phy.c
+++ b/drivers/scsi/libsas/sas_phy.c
@@ -36,8 +36,7 @@ static void sas_phye_loss_of_signal(struct work_struct *work)
container_of(work, struct asd_sas_event, work);
struct asd_sas_phy *phy = ev->phy;
- sas_begin_event(PHYE_LOSS_OF_SIGNAL, &phy->ha->event_lock,
- &phy->phy_events_pending);
+ clear_bit(PHYE_LOSS_OF_SIGNAL, &phy->phy_events_pending);
phy->error = 0;
sas_deform_port(phy, 1);
}
@@ -48,8 +47,7 @@ static void sas_phye_oob_done(struct work_struct *work)
container_of(work, struct asd_sas_event, work);
struct asd_sas_phy *phy = ev->phy;
- sas_begin_event(PHYE_OOB_DONE, &phy->ha->event_lock,
- &phy->phy_events_pending);
+ clear_bit(PHYE_OOB_DONE, &phy->phy_events_pending);
phy->error = 0;
}
@@ -63,8 +61,7 @@ static void sas_phye_oob_error(struct work_struct *work)
struct sas_internal *i =
to_sas_internal(sas_ha->core.shost->transportt);
- sas_begin_event(PHYE_OOB_ERROR, &phy->ha->event_lock,
- &phy->phy_events_pending);
+ clear_bit(PHYE_OOB_ERROR, &phy->phy_events_pending);
sas_deform_port(phy, 1);
@@ -95,8 +92,7 @@ static void sas_phye_spinup_hold(struct work_struct *work)
struct sas_internal *i =
to_sas_internal(sas_ha->core.shost->transportt);
- sas_begin_event(PHYE_SPINUP_HOLD, &phy->ha->event_lock,
- &phy->phy_events_pending);
+ clear_bit(PHYE_SPINUP_HOLD, &phy->phy_events_pending);
phy->error = 0;
i->dft->lldd_control_phy(phy, PHY_FUNC_RELEASE_SPINUP_HOLD, NULL);
diff --git a/drivers/scsi/libsas/sas_port.c b/drivers/scsi/libsas/sas_port.c
index 42fd1f2..a47c7a7 100644
--- a/drivers/scsi/libsas/sas_port.c
+++ b/drivers/scsi/libsas/sas_port.c
@@ -213,8 +213,7 @@ void sas_porte_bytes_dmaed(struct work_struct *work)
container_of(work, struct asd_sas_event, work);
struct asd_sas_phy *phy = ev->phy;
- sas_begin_event(PORTE_BYTES_DMAED, &phy->ha->event_lock,
- &phy->port_events_pending);
+ clear_bit(PORTE_BYTES_DMAED, &phy->port_events_pending);
sas_form_port(phy);
}
@@ -227,8 +226,7 @@ void sas_porte_broadcast_rcvd(struct work_struct *work)
unsigned long flags;
u32 prim;
- sas_begin_event(PORTE_BROADCAST_RCVD, &phy->ha->event_lock,
- &phy->port_events_pending);
+ clear_bit(PORTE_BROADCAST_RCVD, &phy->port_events_pending);
spin_lock_irqsave(&phy->sas_prim_lock, flags);
prim = phy->sas_prim;
@@ -244,8 +242,7 @@ void sas_porte_link_reset_err(struct work_struct *work)
container_of(work, struct asd_sas_event, work);
struct asd_sas_phy *phy = ev->phy;
- sas_begin_event(PORTE_LINK_RESET_ERR, &phy->ha->event_lock,
- &phy->port_events_pending);
+ clear_bit(PORTE_LINK_RESET_ERR, &phy->port_events_pending);
sas_deform_port(phy, 1);
}
@@ -256,8 +253,7 @@ void sas_porte_timer_event(struct work_struct *work)
container_of(work, struct asd_sas_event, work);
struct asd_sas_phy *phy = ev->phy;
- sas_begin_event(PORTE_TIMER_EVENT, &phy->ha->event_lock,
- &phy->port_events_pending);
+ clear_bit(PORTE_TIMER_EVENT, &phy->port_events_pending);
sas_deform_port(phy, 1);
}
@@ -268,8 +264,7 @@ void sas_porte_hard_reset(struct work_struct *work)
container_of(work, struct asd_sas_event, work);
struct asd_sas_phy *phy = ev->phy;
- sas_begin_event(PORTE_HARD_RESET, &phy->ha->event_lock,
- &phy->port_events_pending);
+ clear_bit(PORTE_HARD_RESET, &phy->port_events_pending);
sas_deform_port(phy, 1);
}
diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index 7ecb5c1..de63a66 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -215,7 +215,6 @@ struct sas_discovery_event {
};
struct sas_discovery {
- spinlock_t disc_event_lock;
struct sas_discovery_event disc_work[DISC_NUM_EVENTS];
unsigned long pending;
u8 fanout_sas_addr[8];
@@ -272,7 +271,6 @@ struct asd_sas_event {
*/
struct asd_sas_phy {
/* private: */
- /* protected by ha->event_lock */
struct asd_sas_event port_events[PORT_NUM_EVENTS];
struct asd_sas_event phy_events[PHY_NUM_EVENTS];
@@ -337,7 +335,6 @@ enum sas_ha_state {
struct sas_ha_struct {
/* private: */
- spinlock_t event_lock;
struct sas_ha_event ha_events[HA_NUM_EVENTS];
unsigned long pending;
^ permalink raw reply related [flat|nested] 39+ messages in thread* [PATCH 09/24] libsas: remove ata_port.lock management duties from lldds
2011-12-17 2:33 [PATCH 00/24] libsas: eh reworks (ata-eh vs discovery, races, ...) Dan Williams
` (7 preceding siblings ...)
2011-12-17 2:33 ` [PATCH 08/24] libsas: replace event locks with atomic bitops Dan Williams
@ 2011-12-17 2:33 ` Dan Williams
2011-12-17 2:33 ` [PATCH 10/24] libsas: prevent domain rediscovery competing with ata error handling Dan Williams
` (14 subsequent siblings)
23 siblings, 0 replies; 39+ messages in thread
From: Dan Williams @ 2011-12-17 2:33 UTC (permalink / raw)
To: linux-scsi; +Cc: Xiangliang Yu, linux-ide, Jack Wang
Each libsas driver (mvsas, pm8001, and isci) has invented a different
method for managing the ap->lock. The lock is held by the ata
->queuecommand() path. mvsas drops it prior to acquiring any internal
locks which allows it to hold its internal lock across calls to
task->task_done(). This capability is important as it is the only way
the driver can flush task->task_done() instances to guarantee that it no
longer has any in-flight references to a domain_device at
->lldd_dev_gone() time.
Assumes ->queuecommand() is always called with irqs enabled which was
the assumption mvsas was making prior to the conversion.
Cc: Jack Wang <jack_wang@usish.com>
Cc: Xiangliang Yu <yuxiangl@marvell.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/scsi/isci/request.c | 3 +--
drivers/scsi/isci/task.c | 6 ++----
drivers/scsi/isci/task.h | 36 -----------------------------------
drivers/scsi/libsas/sas_ata.c | 31 ++++++++++++++++++------------
drivers/scsi/libsas/sas_scsi_host.c | 6 ++----
drivers/scsi/mvsas/mv_sas.c | 6 ------
drivers/scsi/pm8001/pm8001_sas.c | 6 +-----
7 files changed, 24 insertions(+), 70 deletions(-)
diff --git a/drivers/scsi/isci/request.c b/drivers/scsi/isci/request.c
index 192cb48..83383ef 100644
--- a/drivers/scsi/isci/request.c
+++ b/drivers/scsi/isci/request.c
@@ -3649,8 +3649,7 @@ int isci_request_execute(struct isci_host *ihost, struct isci_remote_device *ide
/* Cause this task to be scheduled in the SCSI error
* handler thread.
*/
- isci_execpath_callback(ihost, task,
- sas_task_abort);
+ sas_task_abort(task);
/* Change the status, since we are holding
* the I/O until it is managed by the SCSI
diff --git a/drivers/scsi/isci/task.c b/drivers/scsi/isci/task.c
index 66ad3dc..5901a0e 100644
--- a/drivers/scsi/isci/task.c
+++ b/drivers/scsi/isci/task.c
@@ -96,8 +96,7 @@ static void isci_task_refuse(struct isci_host *ihost, struct sas_task *task,
__func__, task, response, status);
task->lldd_task = NULL;
-
- isci_execpath_callback(ihost, task, task->task_done);
+ task->task_done(task);
break;
case isci_perform_aborted_io_completion:
@@ -117,8 +116,7 @@ static void isci_task_refuse(struct isci_host *ihost, struct sas_task *task,
"%s: Error - task = %p, response=%d, "
"status=%d\n",
__func__, task, response, status);
-
- isci_execpath_callback(ihost, task, sas_task_abort);
+ sas_task_abort(task);
break;
default:
diff --git a/drivers/scsi/isci/task.h b/drivers/scsi/isci/task.h
index bc78c0a..df8d440 100644
--- a/drivers/scsi/isci/task.h
+++ b/drivers/scsi/isci/task.h
@@ -322,40 +322,4 @@ isci_task_set_completion_status(
return task_notification_selection;
}
-/**
-* isci_execpath_callback() - This function is called from the task
-* execute path when the task needs to callback libsas about the submit-time
-* task failure. The callback occurs either through the task's done function
-* or through sas_task_abort. In the case of regular non-discovery SATA/STP I/O
-* requests, libsas takes the host lock before calling execute task. Therefore
-* in this situation the host lock must be managed before calling the func.
-*
-* @ihost: This parameter is the controller to which the I/O request was sent.
-* @task: This parameter is the I/O request.
-* @func: This parameter is the function to call in the correct context.
-* @status: This parameter is the status code for the completed task.
-*
-*/
-static inline void isci_execpath_callback(struct isci_host *ihost,
- struct sas_task *task,
- void (*func)(struct sas_task *))
-{
- struct domain_device *dev = task->dev;
-
- if (dev_is_sata(dev) && task->uldd_task) {
- unsigned long flags;
-
- /* Since we are still in the submit path, and since
- * libsas takes the host lock on behalf of SATA
- * devices before I/O starts (in the non-discovery case),
- * we need to unlock before we can call the callback function.
- */
- raw_local_irq_save(flags);
- spin_unlock(dev->sata_dev.ap->lock);
- func(task);
- spin_lock(dev->sata_dev.ap->lock);
- raw_local_irq_restore(flags);
- } else
- func(task);
-}
#endif /* !defined(_SCI_TASK_H_) */
diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index 83118d0..0489001 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -166,23 +166,26 @@ qc_already_gone:
static unsigned int sas_ata_qc_issue(struct ata_queued_cmd *qc)
{
- int res;
+ unsigned int si;
+ unsigned int xfer = 0;
struct sas_task *task;
- struct domain_device *dev = qc->ap->private_data;
+ struct scatterlist *sg;
+ int ret = AC_ERR_SYSTEM;
+ struct ata_port *ap = qc->ap;
+ struct domain_device *dev = ap->private_data;
struct sas_ha_struct *sas_ha = dev->port->ha;
struct Scsi_Host *host = sas_ha->core.shost;
struct sas_internal *i = to_sas_internal(host->transportt);
- struct scatterlist *sg;
- unsigned int xfer = 0;
- unsigned int si;
+
+ spin_unlock_irq(ap->lock);
/* If the device fell off, no sense in issuing commands */
if (dev->gone)
- return AC_ERR_SYSTEM;
+ goto out;
task = sas_alloc_task(GFP_ATOMIC);
if (!task)
- return AC_ERR_SYSTEM;
+ goto out;
task->dev = dev;
task->task_proto = SAS_PROTOCOL_STP;
task->task_done = sas_ata_task_done;
@@ -227,21 +230,23 @@ static unsigned int sas_ata_qc_issue(struct ata_queued_cmd *qc)
ASSIGN_SAS_TASK(qc->scsicmd, task);
if (sas_ha->lldd_max_execute_num < 2)
- res = i->dft->lldd_execute_task(task, 1, GFP_ATOMIC);
+ ret = i->dft->lldd_execute_task(task, 1, GFP_ATOMIC);
else
- res = sas_queue_up(task);
+ ret = sas_queue_up(task);
/* Examine */
- if (res) {
- SAS_DPRINTK("lldd_execute_task returned: %d\n", res);
+ if (ret) {
+ SAS_DPRINTK("lldd_execute_task returned: %d\n", ret);
if (qc->scsicmd)
ASSIGN_SAS_TASK(qc->scsicmd, NULL);
sas_free_task(task);
- return AC_ERR_SYSTEM;
+ ret = AC_ERR_SYSTEM;
}
- return 0;
+ out:
+ spin_lock_irq(ap->lock);
+ return ret;
}
static bool sas_ata_qc_fill_rtf(struct ata_queued_cmd *qc)
diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c
index 4636453..8c91afa 100644
--- a/drivers/scsi/libsas/sas_scsi_host.c
+++ b/drivers/scsi/libsas/sas_scsi_host.c
@@ -198,11 +198,9 @@ int sas_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
}
if (dev_is_sata(dev)) {
- unsigned long flags;
-
- spin_lock_irqsave(dev->sata_dev.ap->lock, flags);
+ spin_lock_irq(dev->sata_dev.ap->lock);
res = ata_sas_queuecmd(cmd, dev->sata_dev.ap);
- spin_unlock_irqrestore(dev->sata_dev.ap->lock, flags);
+ spin_unlock_irq(dev->sata_dev.ap->lock);
return res;
}
diff --git a/drivers/scsi/mvsas/mv_sas.c b/drivers/scsi/mvsas/mv_sas.c
index a4884a5..911fc42 100644
--- a/drivers/scsi/mvsas/mv_sas.c
+++ b/drivers/scsi/mvsas/mv_sas.c
@@ -893,9 +893,6 @@ static int mvs_task_exec(struct sas_task *task, const int num, gfp_t gfp_flags,
mvi = ((struct mvs_device *)task->dev->lldd_dev)->mvi_info;
- if ((dev->dev_type == SATA_DEV) && (dev->sata_dev.ap != NULL))
- spin_unlock_irq(dev->sata_dev.ap->lock);
-
spin_lock_irqsave(&mvi->lock, flags);
rc = mvs_task_prep(task, mvi, is_tmf, tmf, &pass);
if (rc)
@@ -906,9 +903,6 @@ static int mvs_task_exec(struct sas_task *task, const int num, gfp_t gfp_flags,
(MVS_CHIP_SLOT_SZ - 1));
spin_unlock_irqrestore(&mvi->lock, flags);
- if ((dev->dev_type == SATA_DEV) && (dev->sata_dev.ap != NULL))
- spin_lock_irq(dev->sata_dev.ap->lock);
-
return rc;
}
diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c
index fb3dc99..0ba1f723 100644
--- a/drivers/scsi/pm8001/pm8001_sas.c
+++ b/drivers/scsi/pm8001/pm8001_sas.c
@@ -340,7 +340,7 @@ static int pm8001_task_exec(struct sas_task *task, const int num,
struct pm8001_ccb_info *ccb;
u32 tag = 0xdeadbeef, rc, n_elem = 0;
u32 n = num;
- unsigned long flags = 0, flags_libsas = 0;
+ unsigned long flags = 0;
if (!dev->port) {
struct task_status_struct *tsm = &t->task_status;
@@ -364,11 +364,7 @@ static int pm8001_task_exec(struct sas_task *task, const int num,
ts->stat = SAS_PHY_DOWN;
spin_unlock_irqrestore(&pm8001_ha->lock, flags);
- spin_unlock_irqrestore(dev->sata_dev.ap->lock,
- flags_libsas);
t->task_done(t);
- spin_lock_irqsave(dev->sata_dev.ap->lock,
- flags_libsas);
spin_lock_irqsave(&pm8001_ha->lock, flags);
if (n > 1)
t = list_entry(t->list.next,
^ permalink raw reply related [flat|nested] 39+ messages in thread* [PATCH 10/24] libsas: prevent domain rediscovery competing with ata error handling
2011-12-17 2:33 [PATCH 00/24] libsas: eh reworks (ata-eh vs discovery, races, ...) Dan Williams
` (8 preceding siblings ...)
2011-12-17 2:33 ` [PATCH 09/24] libsas: remove ata_port.lock management duties from lldds Dan Williams
@ 2011-12-17 2:33 ` Dan Williams
2011-12-17 2:34 ` [PATCH 11/24] libsas: use ->set_dmamode to notify lldds of NCQ parameters Dan Williams
` (13 subsequent siblings)
23 siblings, 0 replies; 39+ messages in thread
From: Dan Williams @ 2011-12-17 2:33 UTC (permalink / raw)
To: linux-scsi; +Cc: linux-ide
libata error handling provides for a timeout for link recovery. libsas
must not rescan for previously known devices in this interval otherwise
it may remove a device that is simply waiting for its link to recover.
Let libata-eh make the determination of when the link is stable and
prevent libsas (host workqueue) from taking action while this
determination is pending.
This adds a new cleanup state for domain devices to libsas
'allocated-not-probed'. In this state dev->rphy points to a rphy that
is known to have been through a sas_rphy_add() event. At
sas_unregister_dev() time check if this device is still pending probe
and cleanup accordingly.
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/scsi/libsas/sas_ata.c | 45 +++++++++++++++++++++++++++++++++---
drivers/scsi/libsas/sas_discover.c | 25 ++++++++++++++++++--
drivers/scsi/libsas/sas_expander.c | 5 ++--
drivers/scsi/libsas/sas_init.c | 1 +
drivers/scsi/libsas/sas_internal.h | 1 +
drivers/scsi/libsas/sas_port.c | 1 +
include/scsi/libsas.h | 8 +++++-
include/scsi/sas_ata.h | 5 ++++
8 files changed, 79 insertions(+), 12 deletions(-)
diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index 0489001..4b91c74 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -753,6 +753,35 @@ static int sas_discover_sata_pm(struct domain_device *dev)
return -ENODEV;
}
+void sas_probe_sata(struct work_struct *work)
+{
+ struct domain_device *dev, *n;
+ struct sas_discovery_event *ev =
+ container_of(work, struct sas_discovery_event, work);
+ struct asd_sas_port *port = ev->port;
+
+ clear_bit(DISCE_PROBE, &port->disc.pending);
+
+ list_for_each_entry_safe(dev, n, &port->disco_list, disco_list_node) {
+ int err;
+
+ spin_lock_irq(&port->dev_list_lock);
+ list_add_tail(&dev->dev_list_node, &port->dev_list);
+ spin_unlock_irq(&port->dev_list_lock);
+
+ err = sas_rphy_add(dev->rphy);
+
+ if (err) {
+ SAS_DPRINTK("%s: for %s device %16llx returned %d\n",
+ __func__, dev->parent ? "exp-attached" :
+ "direct-attached",
+ SAS_ADDR(dev->sas_addr), err);
+ sas_unregister_dev(port, dev);
+ } else
+ list_del_init(&dev->disco_list_node);
+ }
+}
+
/**
* sas_discover_sata -- discover an STP/SATA domain device
* @dev: pointer to struct domain_device of interest
@@ -789,10 +818,15 @@ int sas_discover_sata(struct domain_device *dev)
break;
}
sas_notify_lldd_dev_gone(dev);
- if (!res) {
- sas_notify_lldd_dev_found(dev);
- res = sas_rphy_add(dev->rphy);
- }
+
+ if (res)
+ return res;
+
+ res = sas_notify_lldd_dev_found(dev);
+ if (res)
+ return res;
+
+ sas_discover_event(dev->port, DISCE_PROBE);
return res;
}
@@ -800,7 +834,9 @@ int sas_discover_sata(struct domain_device *dev)
void sas_ata_strategy_handler(struct Scsi_Host *shost)
{
struct scsi_device *sdev;
+ struct sas_ha_struct *sas_ha = SHOST_TO_SAS_HA(shost);
+ mutex_lock(&sas_ha->eh_mutex);
shost_for_each_device(sdev, shost) {
struct domain_device *ddev = sdev_to_domain_dev(sdev);
struct ata_port *ap = ddev->sata_dev.ap;
@@ -811,6 +847,7 @@ void sas_ata_strategy_handler(struct Scsi_Host *shost)
ata_port_printk(ap, KERN_DEBUG, "sas eh calling libata port error handler");
ata_scsi_port_error_handler(shost, ap);
}
+ mutex_unlock(&sas_ha->eh_mutex);
}
int sas_ata_timed_out(struct scsi_cmnd *cmd, struct sas_task *task,
diff --git a/drivers/scsi/libsas/sas_discover.c b/drivers/scsi/libsas/sas_discover.c
index ed04118..5fb072c 100644
--- a/drivers/scsi/libsas/sas_discover.c
+++ b/drivers/scsi/libsas/sas_discover.c
@@ -148,9 +148,14 @@ static int sas_get_port_device(struct asd_sas_port *port)
port->disc.max_level = 0;
dev->rphy = rphy;
- spin_lock_irq(&port->dev_list_lock);
- list_add_tail(&dev->dev_list_node, &port->dev_list);
- spin_unlock_irq(&port->dev_list_lock);
+
+ if (dev_is_sata(dev))
+ list_add_tail(&dev->disco_list_node, &port->disco_list);
+ else {
+ spin_lock_irq(&port->dev_list_lock);
+ list_add_tail(&dev->dev_list_node, &port->dev_list);
+ spin_unlock_irq(&port->dev_list_lock);
+ }
return 0;
}
@@ -257,6 +262,12 @@ static void sas_unregister_common_dev(struct asd_sas_port *port, struct domain_d
void sas_unregister_dev(struct asd_sas_port *port, struct domain_device *dev)
{
+ if (!list_empty(&dev->disco_list_node)) {
+ list_del_init(&dev->disco_list_node);
+ sas_rphy_free(dev->rphy);
+ dev->rphy = NULL;
+ }
+
if (dev->rphy) {
sas_remove_children(&dev->rphy->dev);
sas_rphy_delete(dev->rphy);
@@ -271,6 +282,8 @@ void sas_unregister_domain_devices(struct asd_sas_port *port)
list_for_each_entry_safe_reverse(dev, n, &port->dev_list, dev_list_node)
sas_unregister_dev(port, dev);
+ list_for_each_entry_safe(dev, n, &port->disco_list, disco_list_node)
+ sas_unregister_dev(port, dev);
port->port->rphy = NULL;
@@ -335,6 +348,7 @@ static void sas_discover_domain(struct work_struct *work)
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);
spin_unlock_irq(&port->dev_list_lock);
@@ -358,8 +372,12 @@ static void sas_revalidate_domain(struct work_struct *work)
SAS_DPRINTK("REVALIDATING DOMAIN on port %d, pid:%d\n", port->id,
task_pid_nr(current));
+
+ /* prevent rediscovery from finding sata links in recovery */
+ mutex_lock(&port->ha->eh_mutex);
if (port->port_dev)
res = sas_ex_revalidate_domain(port->port_dev);
+ mutex_unlock(&port->ha->eh_mutex);
SAS_DPRINTK("done REVALIDATING DOMAIN on port %d, pid:%d, res 0x%x\n",
port->id, task_pid_nr(current), res);
@@ -395,6 +413,7 @@ void sas_init_disc(struct sas_discovery *disc, struct asd_sas_port *port)
static const work_func_t sas_event_fns[DISC_NUM_EVENTS] = {
[DISCE_DISCOVER_DOMAIN] = sas_discover_domain,
[DISCE_REVALIDATE_DOMAIN] = sas_revalidate_domain,
+ [DISCE_PROBE] = sas_probe_sata,
};
disc->pending = 0;
diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index 15d2239..c3846cf 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -704,9 +704,7 @@ static struct domain_device *sas_ex_discover_end_dev(
child->rphy = rphy;
- spin_lock_irq(&parent->port->dev_list_lock);
- list_add_tail(&child->dev_list_node, &parent->port->dev_list);
- spin_unlock_irq(&parent->port->dev_list_lock);
+ list_add_tail(&child->disco_list_node, &parent->port->disco_list);
res = sas_discover_sata(child);
if (res) {
@@ -756,6 +754,7 @@ static struct domain_device *sas_ex_discover_end_dev(
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);
spin_unlock_irq(&parent->port->dev_list_lock);
diff --git a/drivers/scsi/libsas/sas_init.c b/drivers/scsi/libsas/sas_init.c
index a435876..55e34c6 100644
--- a/drivers/scsi/libsas/sas_init.c
+++ b/drivers/scsi/libsas/sas_init.c
@@ -104,6 +104,7 @@ int sas_register_ha(struct sas_ha_struct *sas_ha)
{
int error = 0;
+ mutex_init(&sas_ha->eh_mutex);
spin_lock_init(&sas_ha->phy_port_lock);
sas_hash_addr(sas_ha->hashed_sas_addr, sas_ha->sas_addr);
diff --git a/drivers/scsi/libsas/sas_internal.h b/drivers/scsi/libsas/sas_internal.h
index 7fe4ede..128c941 100644
--- a/drivers/scsi/libsas/sas_internal.h
+++ b/drivers/scsi/libsas/sas_internal.h
@@ -152,6 +152,7 @@ static inline struct domain_device *sas_alloc_device(void)
if (dev) {
INIT_LIST_HEAD(&dev->siblings);
INIT_LIST_HEAD(&dev->dev_list_node);
+ INIT_LIST_HEAD(&dev->disco_list_node);
kref_init(&dev->kref);
}
return dev;
diff --git a/drivers/scsi/libsas/sas_port.c b/drivers/scsi/libsas/sas_port.c
index a47c7a7..df230f1 100644
--- a/drivers/scsi/libsas/sas_port.c
+++ b/drivers/scsi/libsas/sas_port.c
@@ -277,6 +277,7 @@ static void sas_init_port(struct asd_sas_port *port,
memset(port, 0, sizeof(*port));
port->id = i;
INIT_LIST_HEAD(&port->dev_list);
+ INIT_LIST_HEAD(&port->disco_list);
spin_lock_init(&port->phy_list_lock);
INIT_LIST_HEAD(&port->phy_list);
port->ha = sas_ha;
diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index de63a66..f1e4f2d 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -86,7 +86,8 @@ enum discover_event {
DISCE_DISCOVER_DOMAIN = 0U,
DISCE_REVALIDATE_DOMAIN = 1,
DISCE_PORT_GONE = 2,
- DISC_NUM_EVENTS = 3,
+ DISCE_PROBE = 3,
+ DISC_NUM_EVENTS = 4,
};
/* ---------- Expander Devices ---------- */
@@ -188,6 +189,7 @@ struct domain_device {
struct asd_sas_port *port; /* shortcut to root of the tree */
struct list_head dev_list_node;
+ struct list_head disco_list_node;
enum sas_protocol iproto;
enum sas_protocol tproto;
@@ -223,7 +225,6 @@ struct sas_discovery {
int max_level;
};
-
/* The port struct is Class:RW, driver:RO */
struct asd_sas_port {
/* private: */
@@ -233,6 +234,7 @@ struct asd_sas_port {
struct domain_device *port_dev;
spinlock_t dev_list_lock;
struct list_head dev_list;
+ struct list_head disco_list;
enum sas_linkrate linkrate;
struct sas_phy *phy;
@@ -341,6 +343,8 @@ struct sas_ha_struct {
enum sas_ha_state state;
spinlock_t state_lock;
+ struct mutex eh_mutex;
+
struct scsi_core core;
/* public: */
diff --git a/include/scsi/sas_ata.h b/include/scsi/sas_ata.h
index 7d5013f..557fc9a 100644
--- a/include/scsi/sas_ata.h
+++ b/include/scsi/sas_ata.h
@@ -45,6 +45,7 @@ int sas_ata_timed_out(struct scsi_cmnd *cmd, struct sas_task *task,
enum blk_eh_timer_return *rtn);
int sas_ata_eh(struct Scsi_Host *shost, struct list_head *work_q,
struct list_head *done_q);
+void sas_probe_sata(struct work_struct *work);
#else
@@ -78,6 +79,10 @@ static inline int sas_ata_eh(struct Scsi_Host *shost, struct list_head *work_q,
return 0;
}
+static inline void sas_probe_sata(struct work_struct *work)
+{
+}
+
#endif
#endif /* _SAS_ATA_H_ */
^ permalink raw reply related [flat|nested] 39+ messages in thread* [PATCH 11/24] libsas: use ->set_dmamode to notify lldds of NCQ parameters
2011-12-17 2:33 [PATCH 00/24] libsas: eh reworks (ata-eh vs discovery, races, ...) Dan Williams
` (9 preceding siblings ...)
2011-12-17 2:33 ` [PATCH 10/24] libsas: prevent domain rediscovery competing with ata error handling Dan Williams
@ 2011-12-17 2:34 ` Dan Williams
2011-12-17 2:34 ` [PATCH 12/24] libsas: kill invocation of scsi_eh_finish_cmd from sas_ata_task_done Dan Williams
` (12 subsequent siblings)
23 siblings, 0 replies; 39+ messages in thread
From: Dan Williams @ 2011-12-17 2:34 UTC (permalink / raw)
To: linux-scsi; +Cc: Xiangliang Yu, linux-ide, Luben Tuikov, Jack Wang
sas_discover_sata() notifies lldds of sata devices twice. Once to allow
the 'identify' to be sent, and a second time to allow aic94xx (the only
libsas driver that cares about sata_dev.identify) to setup NCQ
parameters before the device becomes known to the midlayer. Replace
this double notification and intervening 'identify' with an explicit
->lldd_ata_set_dmamode notification. With this change all ata internal
commands are issued by libata, so we no longer need sas_issue_ata_cmd().
The data from the identify command only needs to be cached in one
location so ata_device.id replaces domain_device.sata_dev.identify.
Cc: Jack Wang <jack_wang@usish.com>
Cc: Xiangliang Yu <yuxiangl@marvell.com>
Cc: Luben Tuikov <ltuikov@yahoo.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/scsi/aic94xx/aic94xx.h | 2
drivers/scsi/aic94xx/aic94xx_dev.c | 38 +++-
drivers/scsi/aic94xx/aic94xx_init.c | 2
drivers/scsi/libsas/sas_ata.c | 324 ++---------------------------------
drivers/scsi/libsas/sas_discover.c | 5 -
include/scsi/libsas.h | 4
6 files changed, 49 insertions(+), 326 deletions(-)
diff --git a/drivers/scsi/aic94xx/aic94xx.h b/drivers/scsi/aic94xx/aic94xx.h
index 2863a9d..66cda66 100644
--- a/drivers/scsi/aic94xx/aic94xx.h
+++ b/drivers/scsi/aic94xx/aic94xx.h
@@ -80,6 +80,8 @@ void asd_invalidate_edb(struct asd_ascb *ascb, int edb_id);
int asd_execute_task(struct sas_task *, int num, gfp_t gfp_flags);
+void asd_set_dmamode(struct domain_device *dev);
+
/* ---------- TMFs ---------- */
int asd_abort_task(struct sas_task *);
int asd_abort_task_set(struct domain_device *, u8 *lun);
diff --git a/drivers/scsi/aic94xx/aic94xx_dev.c b/drivers/scsi/aic94xx/aic94xx_dev.c
index 2e2ddec..64136c56 100644
--- a/drivers/scsi/aic94xx/aic94xx_dev.c
+++ b/drivers/scsi/aic94xx/aic94xx_dev.c
@@ -109,26 +109,37 @@ static int asd_init_sata_tag_ddb(struct domain_device *dev)
return 0;
}
-static int asd_init_sata(struct domain_device *dev)
+void asd_set_dmamode(struct domain_device *dev)
{
struct asd_ha_struct *asd_ha = dev->port->ha->lldd_ha;
+ struct ata_device *ata_dev = sas_to_ata_dev(dev);
int ddb = (int) (unsigned long) dev->lldd_dev;
u32 qdepth = 0;
- int res = 0;
- asd_ddbsite_write_word(asd_ha, ddb, ATA_CMD_SCBPTR, 0xFFFF);
- if ((dev->dev_type == SATA_DEV || dev->dev_type == SATA_PM_PORT) &&
- dev->sata_dev.identify_device &&
- dev->sata_dev.identify_device[10] != 0) {
- u16 w75 = le16_to_cpu(dev->sata_dev.identify_device[75]);
- u16 w76 = le16_to_cpu(dev->sata_dev.identify_device[76]);
-
- if (w76 & 0x100) /* NCQ? */
- qdepth = (w75 & 0x1F) + 1;
+ if (dev->dev_type == SATA_DEV || dev->dev_type == SATA_PM_PORT) {
+ if (ata_id_has_ncq(ata_dev->id))
+ qdepth = ata_id_queue_depth(ata_dev->id);
asd_ddbsite_write_dword(asd_ha, ddb, SATA_TAG_ALLOC_MASK,
(1ULL<<qdepth)-1);
asd_ddbsite_write_byte(asd_ha, ddb, NUM_SATA_TAGS, qdepth);
}
+
+ if (qdepth > 0)
+ if (asd_init_sata_tag_ddb(dev) != 0) {
+ unsigned long flags;
+
+ spin_lock_irqsave(dev->sata_dev.ap->lock, flags);
+ ata_dev->flags |= ATA_DFLAG_NCQ_OFF;
+ spin_unlock_irqrestore(dev->sata_dev.ap->lock, flags);
+ }
+}
+
+static int asd_init_sata(struct domain_device *dev)
+{
+ struct asd_ha_struct *asd_ha = dev->port->ha->lldd_ha;
+ int ddb = (int) (unsigned long) dev->lldd_dev;
+
+ asd_ddbsite_write_word(asd_ha, ddb, ATA_CMD_SCBPTR, 0xFFFF);
if (dev->dev_type == SATA_DEV || dev->dev_type == SATA_PM ||
dev->dev_type == SATA_PM_PORT) {
struct dev_to_host_fis *fis = (struct dev_to_host_fis *)
@@ -136,9 +147,8 @@ static int asd_init_sata(struct domain_device *dev)
asd_ddbsite_write_byte(asd_ha, ddb, SATA_STATUS, fis->status);
}
asd_ddbsite_write_word(asd_ha, ddb, NCQ_DATA_SCB_PTR, 0xFFFF);
- if (qdepth > 0)
- res = asd_init_sata_tag_ddb(dev);
- return res;
+
+ return 0;
}
static int asd_init_target_ddb(struct domain_device *dev)
diff --git a/drivers/scsi/aic94xx/aic94xx_init.c b/drivers/scsi/aic94xx/aic94xx_init.c
index 8db4e72..49a2cff 100644
--- a/drivers/scsi/aic94xx/aic94xx_init.c
+++ b/drivers/scsi/aic94xx/aic94xx_init.c
@@ -1009,6 +1009,8 @@ static struct sas_domain_function_template aic94xx_transport_functions = {
.lldd_clear_nexus_ha = asd_clear_nexus_ha,
.lldd_control_phy = asd_control_phy,
+
+ .lldd_ata_set_dmamode = asd_set_dmamode,
};
static const struct pci_device_id aic94xx_pci_table[] __devinitdata = {
diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index 4b91c74..dd35a98 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -362,6 +362,17 @@ static void sas_ata_post_internal(struct ata_queued_cmd *qc)
}
}
+
+static void sas_ata_set_dmamode(struct ata_port *ap, struct ata_device *ata_dev)
+{
+ struct domain_device *dev = ap->private_data;
+ struct sas_internal *i =
+ to_sas_internal(dev->port->ha->core.shost->transportt);
+
+ if (i->dft->lldd_ata_set_dmamode)
+ i->dft->lldd_ata_set_dmamode(dev);
+}
+
static struct ata_port_operations sas_sata_ops = {
.prereset = ata_std_prereset,
.softreset = sas_ata_soft_reset,
@@ -375,6 +386,7 @@ static struct ata_port_operations sas_sata_ops = {
.qc_fill_rtf = sas_ata_qc_fill_rtf,
.port_start = ata_sas_port_start,
.port_stop = ata_sas_port_stop,
+ .set_dmamode = sas_ata_set_dmamode,
};
static struct ata_port_info sata_port_info = {
@@ -437,163 +449,6 @@ void sas_ata_task_abort(struct sas_task *task)
complete(waiting);
}
-static void sas_task_timedout(unsigned long _task)
-{
- struct sas_task *task = (void *) _task;
- unsigned long flags;
-
- spin_lock_irqsave(&task->task_state_lock, flags);
- if (!(task->task_state_flags & SAS_TASK_STATE_DONE))
- task->task_state_flags |= SAS_TASK_STATE_ABORTED;
- spin_unlock_irqrestore(&task->task_state_lock, flags);
-
- complete(&task->completion);
-}
-
-static void sas_disc_task_done(struct sas_task *task)
-{
- if (!del_timer(&task->timer))
- return;
- complete(&task->completion);
-}
-
-#define SAS_DEV_TIMEOUT 10
-
-/**
- * sas_execute_task -- Basic task processing for discovery
- * @task: the task to be executed
- * @buffer: pointer to buffer to do I/O
- * @size: size of @buffer
- * @dma_dir: DMA direction. DMA_xxx
- */
-static int sas_execute_task(struct sas_task *task, void *buffer, int size,
- enum dma_data_direction dma_dir)
-{
- int res = 0;
- struct scatterlist *scatter = NULL;
- struct task_status_struct *ts = &task->task_status;
- int num_scatter = 0;
- int retries = 0;
- struct sas_internal *i =
- to_sas_internal(task->dev->port->ha->core.shost->transportt);
-
- if (dma_dir != DMA_NONE) {
- scatter = kzalloc(sizeof(*scatter), GFP_KERNEL);
- if (!scatter)
- goto out;
-
- sg_init_one(scatter, buffer, size);
- num_scatter = 1;
- }
-
- task->task_proto = task->dev->tproto;
- task->scatter = scatter;
- task->num_scatter = num_scatter;
- task->total_xfer_len = size;
- task->data_dir = dma_dir;
- task->task_done = sas_disc_task_done;
- if (dma_dir != DMA_NONE &&
- sas_protocol_ata(task->task_proto)) {
- task->num_scatter = dma_map_sg(task->dev->port->ha->dev,
- task->scatter,
- task->num_scatter,
- task->data_dir);
- }
-
- for (retries = 0; retries < 5; retries++) {
- task->task_state_flags = SAS_TASK_STATE_PENDING;
- init_completion(&task->completion);
-
- task->timer.data = (unsigned long) task;
- task->timer.function = sas_task_timedout;
- task->timer.expires = jiffies + SAS_DEV_TIMEOUT*HZ;
- add_timer(&task->timer);
-
- res = i->dft->lldd_execute_task(task, 1, GFP_KERNEL);
- if (res) {
- del_timer(&task->timer);
- SAS_DPRINTK("executing SAS discovery task failed:%d\n",
- res);
- goto ex_err;
- }
- wait_for_completion(&task->completion);
- res = -ECOMM;
- if (task->task_state_flags & SAS_TASK_STATE_ABORTED) {
- int res2;
- SAS_DPRINTK("task aborted, flags:0x%x\n",
- task->task_state_flags);
- res2 = i->dft->lldd_abort_task(task);
- SAS_DPRINTK("came back from abort task\n");
- if (!(task->task_state_flags & SAS_TASK_STATE_DONE)) {
- if (res2 == TMF_RESP_FUNC_COMPLETE)
- continue; /* Retry the task */
- else
- goto ex_err;
- }
- }
- if (task->task_status.stat == SAM_STAT_BUSY ||
- task->task_status.stat == SAM_STAT_TASK_SET_FULL ||
- task->task_status.stat == SAS_QUEUE_FULL) {
- SAS_DPRINTK("task: q busy, sleeping...\n");
- schedule_timeout_interruptible(HZ);
- } else if (task->task_status.stat == SAM_STAT_CHECK_CONDITION) {
- struct scsi_sense_hdr shdr;
-
- if (!scsi_normalize_sense(ts->buf, ts->buf_valid_size,
- &shdr)) {
- SAS_DPRINTK("couldn't normalize sense\n");
- continue;
- }
- if ((shdr.sense_key == 6 && shdr.asc == 0x29) ||
- (shdr.sense_key == 2 && shdr.asc == 4 &&
- shdr.ascq == 1)) {
- SAS_DPRINTK("device %016llx LUN: %016llx "
- "powering up or not ready yet, "
- "sleeping...\n",
- SAS_ADDR(task->dev->sas_addr),
- SAS_ADDR(task->ssp_task.LUN));
-
- schedule_timeout_interruptible(5*HZ);
- } else if (shdr.sense_key == 1) {
- res = 0;
- break;
- } else if (shdr.sense_key == 5) {
- break;
- } else {
- SAS_DPRINTK("dev %016llx LUN: %016llx "
- "sense key:0x%x ASC:0x%x ASCQ:0x%x"
- "\n",
- SAS_ADDR(task->dev->sas_addr),
- SAS_ADDR(task->ssp_task.LUN),
- shdr.sense_key,
- shdr.asc, shdr.ascq);
- }
- } else if (task->task_status.resp != SAS_TASK_COMPLETE ||
- task->task_status.stat != SAM_STAT_GOOD) {
- SAS_DPRINTK("task finished with resp:0x%x, "
- "stat:0x%x\n",
- task->task_status.resp,
- task->task_status.stat);
- goto ex_err;
- } else {
- res = 0;
- break;
- }
- }
-ex_err:
- if (dma_dir != DMA_NONE) {
- if (sas_protocol_ata(task->task_proto))
- dma_unmap_sg(task->dev->port->ha->dev,
- task->scatter, task->num_scatter,
- task->data_dir);
- kfree(scatter);
- }
-out:
- return res;
-}
-
-/* ---------- SATA ---------- */
-
static void sas_get_ata_command_set(struct domain_device *dev)
{
struct dev_to_host_fis *fis =
@@ -637,122 +492,6 @@ static void sas_get_ata_command_set(struct domain_device *dev)
dev->sata_dev.command_set = ATAPI_COMMAND_SET;
}
-/**
- * sas_issue_ata_cmd -- Basic SATA command processing for discovery
- * @dev: the device to send the command to
- * @command: the command register
- * @features: the features register
- * @buffer: pointer to buffer to do I/O
- * @size: size of @buffer
- * @dma_dir: DMA direction. DMA_xxx
- */
-static int sas_issue_ata_cmd(struct domain_device *dev, u8 command,
- u8 features, void *buffer, int size,
- enum dma_data_direction dma_dir)
-{
- int res = 0;
- struct sas_task *task;
- struct dev_to_host_fis *d2h_fis = (struct dev_to_host_fis *)
- &dev->frame_rcvd[0];
-
- res = -ENOMEM;
- task = sas_alloc_task(GFP_KERNEL);
- if (!task)
- goto out;
-
- task->dev = dev;
-
- task->ata_task.fis.fis_type = 0x27;
- task->ata_task.fis.command = command;
- task->ata_task.fis.features = features;
- task->ata_task.fis.device = d2h_fis->device;
- task->ata_task.retry_count = 1;
-
- res = sas_execute_task(task, buffer, size, dma_dir);
-
- sas_free_task(task);
-out:
- return res;
-}
-
-#define ATA_IDENTIFY_DEV 0xEC
-#define ATA_IDENTIFY_PACKET_DEV 0xA1
-#define ATA_SET_FEATURES 0xEF
-#define ATA_FEATURE_PUP_STBY_SPIN_UP 0x07
-
-/**
- * sas_discover_sata_dev -- discover a STP/SATA device (SATA_DEV)
- * @dev: STP/SATA device of interest (ATA/ATAPI)
- *
- * The LLDD has already been notified of this device, so that we can
- * send FISes to it. Here we try to get IDENTIFY DEVICE or IDENTIFY
- * PACKET DEVICE, if ATAPI device, so that the LLDD can fine-tune its
- * performance for this device.
- */
-static int sas_discover_sata_dev(struct domain_device *dev)
-{
- int res;
- __le16 *identify_x;
- u8 command;
-
- identify_x = kzalloc(512, GFP_KERNEL);
- if (!identify_x)
- return -ENOMEM;
-
- if (dev->sata_dev.command_set == ATA_COMMAND_SET) {
- dev->sata_dev.identify_device = identify_x;
- command = ATA_IDENTIFY_DEV;
- } else {
- dev->sata_dev.identify_packet_device = identify_x;
- command = ATA_IDENTIFY_PACKET_DEV;
- }
-
- res = sas_issue_ata_cmd(dev, command, 0, identify_x, 512,
- DMA_FROM_DEVICE);
- if (res)
- goto out_err;
-
- /* lives on the media? */
- if (le16_to_cpu(identify_x[0]) & 4) {
- /* incomplete response */
- SAS_DPRINTK("sending SET FEATURE/PUP_STBY_SPIN_UP to "
- "dev %llx\n", SAS_ADDR(dev->sas_addr));
- if (!(identify_x[83] & cpu_to_le16(1<<6)))
- goto cont1;
- res = sas_issue_ata_cmd(dev, ATA_SET_FEATURES,
- ATA_FEATURE_PUP_STBY_SPIN_UP,
- NULL, 0, DMA_NONE);
- if (res)
- goto cont1;
-
- schedule_timeout_interruptible(5*HZ); /* More time? */
- res = sas_issue_ata_cmd(dev, command, 0, identify_x, 512,
- DMA_FROM_DEVICE);
- if (res)
- goto out_err;
- }
-cont1:
- /* XXX Hint: register this SATA device with SATL.
- When this returns, dev->sata_dev->lu is alive and
- present.
- sas_satl_register_dev(dev);
- */
-
- sas_fill_in_rphy(dev, dev->rphy);
-
- return 0;
-out_err:
- dev->sata_dev.identify_packet_device = NULL;
- dev->sata_dev.identify_device = NULL;
- kfree(identify_x);
- return res;
-}
-
-static int sas_discover_sata_pm(struct domain_device *dev)
-{
- return -ENODEV;
-}
-
void sas_probe_sata(struct work_struct *work)
{
struct domain_device *dev, *n;
@@ -786,49 +525,26 @@ void sas_probe_sata(struct work_struct *work)
* sas_discover_sata -- discover an STP/SATA domain device
* @dev: pointer to struct domain_device of interest
*
- * First we notify the LLDD of this device, so we can send frames to
- * it. Then depending on the type of device we call the appropriate
- * discover functions. Once device discover is done, we notify the
- * LLDD so that it can fine-tune its parameters for the device, by
- * removing it and then adding it. That is, the second time around,
- * the driver would have certain fields, that it is looking at, set.
- * Finally we initialize the kobj so that the device can be added to
- * the system at registration time. Devices directly attached to a HA
- * port, have no parents. All other devices do, and should have their
- * "parent" pointer set appropriately before calling this function.
+ * Devices directly attached to a HA port, have no parents. All other
+ * devices do, and should have their "parent" pointer set appropriately
+ * before calling this function.
*/
int sas_discover_sata(struct domain_device *dev)
{
int res;
- sas_get_ata_command_set(dev);
-
- res = sas_notify_lldd_dev_found(dev);
- if (res)
- return res;
-
- switch (dev->dev_type) {
- case SATA_DEV:
- res = sas_discover_sata_dev(dev);
- break;
- case SATA_PM:
- res = sas_discover_sata_pm(dev);
- break;
- default:
- break;
- }
- sas_notify_lldd_dev_gone(dev);
+ if (dev->dev_type == SATA_PM)
+ return -ENODEV;
- if (res)
- return res;
+ sas_get_ata_command_set(dev);
+ sas_fill_in_rphy(dev, dev->rphy);
res = sas_notify_lldd_dev_found(dev);
if (res)
return res;
sas_discover_event(dev->port, DISCE_PROBE);
-
- return res;
+ return 0;
}
void sas_ata_strategy_handler(struct Scsi_Host *shost)
diff --git a/drivers/scsi/libsas/sas_discover.c b/drivers/scsi/libsas/sas_discover.c
index 5fb072c..3f41ab7 100644
--- a/drivers/scsi/libsas/sas_discover.c
+++ b/drivers/scsi/libsas/sas_discover.c
@@ -237,11 +237,6 @@ void sas_free_device(struct kref *kref)
if (dev->dev_type == EDGE_DEV || dev->dev_type == FANOUT_DEV)
kfree(dev->ex_dev.ex_phy);
- if (dev_is_sata(dev)) {
- kfree(dev->sata_dev.identify_device);
- kfree(dev->sata_dev.identify_packet_device);
- }
-
kfree(dev);
}
diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index f1e4f2d..2ff089b 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -163,9 +163,6 @@ enum ata_command_set {
struct sata_device {
enum ata_command_set command_set;
struct smp_resp rps_resp; /* report_phy_sata_resp */
- __le16 *identify_device;
- __le16 *identify_packet_device;
-
u8 port_no; /* port number, if this is a PM (Port) */
struct list_head children; /* PM Ports if this is a PM */
@@ -600,6 +597,7 @@ struct sas_domain_function_template {
int (*lldd_clear_task_set)(struct domain_device *, u8 *lun);
int (*lldd_I_T_nexus_reset)(struct domain_device *);
int (*lldd_ata_soft_reset)(struct domain_device *);
+ void (*lldd_ata_set_dmamode)(struct domain_device *);
int (*lldd_lu_reset)(struct domain_device *, u8 *lun);
int (*lldd_query_task)(struct sas_task *);
^ permalink raw reply related [flat|nested] 39+ messages in thread* [PATCH 12/24] libsas: kill invocation of scsi_eh_finish_cmd from sas_ata_task_done
2011-12-17 2:33 [PATCH 00/24] libsas: eh reworks (ata-eh vs discovery, races, ...) Dan Williams
` (10 preceding siblings ...)
2011-12-17 2:34 ` [PATCH 11/24] libsas: use ->set_dmamode to notify lldds of NCQ parameters Dan Williams
@ 2011-12-17 2:34 ` Dan Williams
2011-12-17 2:34 ` [PATCH 13/24] libsas: close error handling vs sas_ata_task_done() race Dan Williams
` (11 subsequent siblings)
23 siblings, 0 replies; 39+ messages in thread
From: Dan Williams @ 2011-12-17 2:34 UTC (permalink / raw)
To: linux-scsi; +Cc: linux-ide, Darrick J. Wong
Prior to the conversion to the new-style libata-eh sas_ata_task_done()
may have been the last opportunity to clean up the scmd, but now
libata-eh explicitly handles this case. It also races against sas-eh.
If a lldd completes a task after SAS_TASK_STATE_ABORTED is set it could
trigger a spurious decrement of shost->host_failed. Current lldds have
the band-aid of checking SAS_TASK_STATE_ABORTED before calling
->task_done(), but better to just let the scmds escalate to libata for
race free cleanup.
Cc: Darrick J. Wong <djwong@us.ibm.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/scsi/libsas/sas_ata.c | 14 --------------
1 files changed, 0 insertions(+), 14 deletions(-)
diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index dd35a98..649b04b 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -145,20 +145,6 @@ static void sas_ata_task_done(struct sas_task *task)
ata_qc_complete(qc);
spin_unlock_irqrestore(dev->sata_dev.ap->lock, flags);
- /*
- * If the sas_task has an ata qc, a scsi_cmnd and the aborted
- * flag is set, then we must have come in via the libsas EH
- * functions. When we exit this function, we need to put the
- * scsi_cmnd on the list of finished errors. The ata_qc_complete
- * call cleans up the libata side of things but we're protected
- * from the scsi_cmnd going away because the scsi_cmnd is owned
- * by the EH, making libata's call to scsi_done a NOP.
- */
- spin_lock_irqsave(&task->task_state_lock, flags);
- if (qc->scsicmd && task->task_state_flags & SAS_TASK_STATE_ABORTED)
- scsi_eh_finish_cmd(qc->scsicmd, &sas_ha->eh_done_q);
- spin_unlock_irqrestore(&task->task_state_lock, flags);
-
qc_already_gone:
list_del_init(&task->list);
sas_free_task(task);
^ permalink raw reply related [flat|nested] 39+ messages in thread* [PATCH 13/24] libsas: close error handling vs sas_ata_task_done() race
2011-12-17 2:33 [PATCH 00/24] libsas: eh reworks (ata-eh vs discovery, races, ...) Dan Williams
` (11 preceding siblings ...)
2011-12-17 2:34 ` [PATCH 12/24] libsas: kill invocation of scsi_eh_finish_cmd from sas_ata_task_done Dan Williams
@ 2011-12-17 2:34 ` Dan Williams
2011-12-17 2:34 ` [PATCH 14/24] libsas: prevent double completion of scmds from eh Dan Williams
` (10 subsequent siblings)
23 siblings, 0 replies; 39+ messages in thread
From: Dan Williams @ 2011-12-17 2:34 UTC (permalink / raw)
To: linux-scsi; +Cc: linux-ide, Darrick J. Wong
Since sas_ata does not implement ->freeze(), completions for scmds and
internal commands can still arrive concurrent with
ata_scsi_cmd_error_handler() and sas_ata_post_internal() respectively.
By the time either of those is called libata has committed to completing
the qc, and the ATA_PFLAG_FROZEN flag tells sas_ata_task_done() it has
lost the race.
In the sas_ata_post_internal() case we take on the additional
responsibility of freeing the sas_task to close the race with
sas_ata_task_done() freeing the the task while sas_ata_post_internal()
is in the process of invoking ->lldd_abort_task().
Cc: Darrick J. Wong <djwong@us.ibm.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/scsi/libsas/sas_ata.c | 85 +++++++++++++++++++++++++++++++----
drivers/scsi/libsas/sas_scsi_host.c | 44 ------------------
include/scsi/libsas.h | 1
3 files changed, 76 insertions(+), 54 deletions(-)
diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index 649b04b..c989d7f 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -100,15 +100,31 @@ static void sas_ata_task_done(struct sas_task *task)
enum ata_completion_errors ac;
unsigned long flags;
struct ata_link *link;
+ struct ata_port *ap;
if (!qc)
goto qc_already_gone;
- dev = qc->ap->private_data;
+ ap = qc->ap;
+ dev = ap->private_data;
sas_ha = dev->port->ha;
- link = &dev->sata_dev.ap->link;
+ link = &ap->link;
+
+ spin_lock_irqsave(ap->lock, flags);
+ /* check if we lost the race with libata/sas_ata_post_internal() */
+ if (unlikely(ap->pflags & ATA_PFLAG_FROZEN)) {
+ spin_unlock_irqrestore(ap->lock, flags);
+ if (qc->scsicmd)
+ goto qc_already_gone;
+ else {
+ /* if eh is not involved and the port is frozen then the
+ * ata internal abort process has taken responsibility
+ * for this sas_task
+ */
+ return;
+ }
+ }
- spin_lock_irqsave(dev->sata_dev.ap->lock, flags);
if (stat->stat == SAS_PROTO_RESPONSE || stat->stat == SAM_STAT_GOOD ||
((stat->stat == SAM_STAT_CHECK_CONDITION &&
dev->sata_dev.command_set == ATAPI_COMMAND_SET))) {
@@ -143,7 +159,7 @@ static void sas_ata_task_done(struct sas_task *task)
if (qc->scsicmd)
ASSIGN_SAS_TASK(qc->scsicmd, NULL);
ata_qc_complete(qc);
- spin_unlock_irqrestore(dev->sata_dev.ap->lock, flags);
+ spin_unlock_irqrestore(ap->lock, flags);
qc_already_gone:
list_del_init(&task->list);
@@ -320,6 +336,55 @@ static int sas_ata_soft_reset(struct ata_link *link, unsigned int *class,
return ret;
}
+/*
+ * notify the lldd to forget the sas_task for this internal ata command
+ * that bypasses scsi-eh
+ */
+static void sas_ata_internal_abort(struct sas_task *task)
+{
+ struct sas_internal *si =
+ to_sas_internal(task->dev->port->ha->core.shost->transportt);
+ unsigned long flags;
+ int res;
+
+ spin_lock_irqsave(&task->task_state_lock, flags);
+ if (task->task_state_flags & SAS_TASK_STATE_ABORTED ||
+ task->task_state_flags & SAS_TASK_STATE_DONE) {
+ spin_unlock_irqrestore(&task->task_state_lock, flags);
+ SAS_DPRINTK("%s: Task %p already finished.\n", __func__,
+ task);
+ goto out;
+ }
+ task->task_state_flags |= SAS_TASK_STATE_ABORTED;
+ spin_unlock_irqrestore(&task->task_state_lock, flags);
+
+ res = si->dft->lldd_abort_task(task);
+
+ spin_lock_irqsave(&task->task_state_lock, flags);
+ if ((task->task_state_flags & SAS_TASK_STATE_DONE) ||
+ (res == TMF_RESP_FUNC_COMPLETE))
+ {
+ spin_unlock_irqrestore(&task->task_state_lock, flags);
+ goto out;
+ }
+
+ /* XXX we are not prepared to deal with ->lldd_abort_task()
+ * failures. TODO: lldds need to unconditionally forget about
+ * aborted ata tasks, otherwise we (likely) leak the sas task
+ * here
+ */
+ SAS_DPRINTK("%s: Task %p leaked.\n", __func__, task);
+
+ if (!(task->task_state_flags & SAS_TASK_STATE_DONE))
+ task->task_state_flags &= ~SAS_TASK_STATE_ABORTED;
+ spin_unlock_irqrestore(&task->task_state_lock, flags);
+
+ return;
+ out:
+ list_del_init(&task->list);
+ sas_free_task(task);
+}
+
static void sas_ata_post_internal(struct ata_queued_cmd *qc)
{
if (qc->flags & ATA_QCFLAG_FAILED)
@@ -327,10 +392,12 @@ static void sas_ata_post_internal(struct ata_queued_cmd *qc)
if (qc->err_mask) {
/*
- * Find the sas_task and kill it. By this point,
- * libata has decided to kill the qc, so we needn't
- * bother with sas_ata_task_done. But we still
- * ought to abort the task.
+ * Find the sas_task and kill it. By this point, libata
+ * has decided to kill the qc and has frozen the port.
+ * In this state sas_ata_task_done() will no longer free
+ * the sas_task, so we need to notify the lldd (via
+ * ->lldd_abort_task) that the task is dead and free it
+ * ourselves.
*/
struct sas_task *task = qc->lldd_task;
unsigned long flags;
@@ -343,7 +410,7 @@ static void sas_ata_post_internal(struct ata_queued_cmd *qc)
spin_unlock_irqrestore(&task->task_state_lock, flags);
task->uldd_task = NULL;
- __sas_task_abort(task);
+ sas_ata_internal_abort(task);
}
}
}
diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c
index 8c91afa..f32c628 100644
--- a/drivers/scsi/libsas/sas_scsi_host.c
+++ b/drivers/scsi/libsas/sas_scsi_host.c
@@ -963,49 +963,6 @@ void sas_shutdown_queue(struct sas_ha_struct *sas_ha)
}
/*
- * Call the LLDD task abort routine directly. This function is intended for
- * use by upper layers that need to tell the LLDD to abort a task.
- */
-int __sas_task_abort(struct sas_task *task)
-{
- struct sas_internal *si =
- to_sas_internal(task->dev->port->ha->core.shost->transportt);
- unsigned long flags;
- int res;
-
- spin_lock_irqsave(&task->task_state_lock, flags);
- if (task->task_state_flags & SAS_TASK_STATE_ABORTED ||
- task->task_state_flags & SAS_TASK_STATE_DONE) {
- spin_unlock_irqrestore(&task->task_state_lock, flags);
- SAS_DPRINTK("%s: Task %p already finished.\n", __func__,
- task);
- return 0;
- }
- task->task_state_flags |= SAS_TASK_STATE_ABORTED;
- spin_unlock_irqrestore(&task->task_state_lock, flags);
-
- if (!si->dft->lldd_abort_task)
- return -ENODEV;
-
- res = si->dft->lldd_abort_task(task);
-
- spin_lock_irqsave(&task->task_state_lock, flags);
- if ((task->task_state_flags & SAS_TASK_STATE_DONE) ||
- (res == TMF_RESP_FUNC_COMPLETE))
- {
- spin_unlock_irqrestore(&task->task_state_lock, flags);
- task->task_done(task);
- return 0;
- }
-
- if (!(task->task_state_flags & SAS_TASK_STATE_DONE))
- task->task_state_flags &= ~SAS_TASK_STATE_ABORTED;
- spin_unlock_irqrestore(&task->task_state_lock, flags);
-
- return -EAGAIN;
-}
-
-/*
* Tell an upper layer that it needs to initiate an abort for a given task.
* This should only ever be called by an LLDD.
*/
@@ -1103,7 +1060,6 @@ EXPORT_SYMBOL_GPL(sas_slave_configure);
EXPORT_SYMBOL_GPL(sas_change_queue_depth);
EXPORT_SYMBOL_GPL(sas_change_queue_type);
EXPORT_SYMBOL_GPL(sas_bios_param);
-EXPORT_SYMBOL_GPL(__sas_task_abort);
EXPORT_SYMBOL_GPL(sas_task_abort);
EXPORT_SYMBOL_GPL(sas_phy_reset);
EXPORT_SYMBOL_GPL(sas_phy_enable);
diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index 2ff089b..6064f82 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -653,7 +653,6 @@ void sas_unregister_dev(struct asd_sas_port *port, struct domain_device *);
void sas_init_dev(struct domain_device *);
void sas_task_abort(struct sas_task *);
-int __sas_task_abort(struct sas_task *);
int sas_eh_device_reset_handler(struct scsi_cmnd *cmd);
int sas_eh_bus_reset_handler(struct scsi_cmnd *cmd);
^ permalink raw reply related [flat|nested] 39+ messages in thread* [PATCH 14/24] libsas: prevent double completion of scmds from eh
2011-12-17 2:33 [PATCH 00/24] libsas: eh reworks (ata-eh vs discovery, races, ...) Dan Williams
` (12 preceding siblings ...)
2011-12-17 2:34 ` [PATCH 13/24] libsas: close error handling vs sas_ata_task_done() race Dan Williams
@ 2011-12-17 2:34 ` Dan Williams
2011-12-17 13:08 ` Sergei Shtylyov
2011-12-17 13:13 ` Sergei Shtylyov
2011-12-17 2:34 ` [PATCH 15/24] libsas: fix timeout vs completion race Dan Williams
` (9 subsequent siblings)
23 siblings, 2 replies; 39+ messages in thread
From: Dan Williams @ 2011-12-17 2:34 UTC (permalink / raw)
To: linux-scsi; +Cc: linux-ide
We invoke task->task_done() to free the task in the eh case, but at this
point we are prepared for scsi_eh_flush_done_q() to finish off the scmd.
Introduce sas_end_task() to capture the final response status from the
lldd and free the task.
Also take the opportunity to kill this warning.
drivers/scsi/libsas/sas_scsi_host.c: In function ‘sas_end_task’:
drivers/scsi/libsas/sas_scsi_host.c:102:3: warning: case value ‘2’ not in enumerated type ‘enum exec_status’ [-Wswitch]
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/scsi/libsas/sas_scsi_host.c | 61 +++++++++++++++++++----------------
include/scsi/libsas.h | 5 ++-
2 files changed, 37 insertions(+), 29 deletions(-)
diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c
index f32c628..13aee61 100644
--- a/drivers/scsi/libsas/sas_scsi_host.c
+++ b/drivers/scsi/libsas/sas_scsi_host.c
@@ -49,27 +49,12 @@
#include <linux/scatterlist.h>
#include <linux/libata.h>
-/* ---------- SCSI Host glue ---------- */
-
-static void sas_scsi_task_done(struct sas_task *task)
+/* record final status and free the task */
+static void sas_end_task(struct scsi_cmnd *sc, struct sas_task *task)
{
struct task_status_struct *ts = &task->task_status;
- struct scsi_cmnd *sc = task->uldd_task;
int hs = 0, stat = 0;
- if (unlikely(task->task_state_flags & SAS_TASK_STATE_ABORTED)) {
- /* Aborted tasks will be completed by the error handler */
- SAS_DPRINTK("task done but aborted\n");
- return;
- }
-
- if (unlikely(!sc)) {
- SAS_DPRINTK("task_done called with non existing SCSI cmnd!\n");
- list_del_init(&task->list);
- sas_free_task(task);
- return;
- }
-
if (ts->resp == SAS_TASK_UNDELIVERED) {
/* transport error */
hs = DID_NO_CONNECT;
@@ -124,10 +109,32 @@ static void sas_scsi_task_done(struct sas_task *task)
break;
}
}
- ASSIGN_SAS_TASK(sc, NULL);
+
sc->result = (hs << 16) | stat;
+ ASSIGN_SAS_TASK(sc, NULL);
list_del_init(&task->list);
sas_free_task(task);
+}
+
+static void sas_scsi_task_done(struct sas_task *task)
+{
+ struct scsi_cmnd *sc = task->uldd_task;
+
+ if (unlikely(task->task_state_flags & SAS_TASK_STATE_ABORTED)) {
+ /* Aborted tasks will be completed by the error handler */
+ SAS_DPRINTK("task done but aborted\n");
+ return;
+ }
+
+ if (unlikely(!sc)) {
+ SAS_DPRINTK("task_done called with non existing SCSI cmnd!\n");
+ list_del_init(&task->list);
+ sas_free_task(task);
+ return;
+ }
+
+ ASSIGN_SAS_TASK(sc, NULL);
+ sas_end_task(sc, task);
sc->scsi_done(sc);
}
@@ -236,18 +243,16 @@ static void sas_eh_finish_cmd(struct scsi_cmnd *cmd)
struct sas_task *task = TO_SAS_TASK(cmd);
struct sas_ha_struct *sas_ha = SHOST_TO_SAS_HA(cmd->device->host);
- /* remove the aborted task flag to allow the task to be
- * completed now. At this point, we only get called following
- * an actual abort of the task, so we should be guaranteed not
- * to be racing with any completions from the LLD (hence we
- * don't need the task state lock to clear the flag) */
- task->task_state_flags &= ~SAS_TASK_STATE_ABORTED;
- /* Now call task_done. However, task will be free'd after
- * this */
- task->task_done(task);
+ /* At this point, we only get called following an actual abort
+ * of the task, so we should be guaranteed not to be racing with
+ * any completions from the LLD. Task is freed after this.
+ */
+ sas_end_task(cmd, task);
+
/* now finish the command and move it on to the error
* handler done list, this also takes it off the
- * error handler pending list */
+ * error handler pending list.
+ */
scsi_eh_finish_cmd(cmd, &sas_ha->eh_done_q);
}
diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index 6064f82..b6b0b99 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -443,7 +443,10 @@ enum service_response {
};
enum exec_status {
- /* The SAM_STAT_.. codes fit in the lower 6 bits */
+ /* The SAM_STAT_.. codes fit in the lower 6 bits, alias some of
+ * them here to silence 'case value not in enumerated type' warnings
+ */
+ __SAM_STAT_CHECK_CONDITION = SAM_STAT_CHECK_CONDITION,
SAS_DEV_NO_RESPONSE = 0x80,
SAS_DATA_UNDERRUN,
--
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 related [flat|nested] 39+ messages in thread* Re: [PATCH 14/24] libsas: prevent double completion of scmds from eh
2011-12-17 2:34 ` [PATCH 14/24] libsas: prevent double completion of scmds from eh Dan Williams
@ 2011-12-17 13:08 ` Sergei Shtylyov
2011-12-18 19:19 ` Dan Williams
2011-12-17 13:13 ` Sergei Shtylyov
1 sibling, 1 reply; 39+ messages in thread
From: Sergei Shtylyov @ 2011-12-17 13:08 UTC (permalink / raw)
To: Dan Williams; +Cc: linux-scsi, linux-ide
Hello.
On 17-12-2011 6:34, Dan Williams wrote:
> We invoke task->task_done() to free the task in the eh case, but at this
> point we are prepared for scsi_eh_flush_done_q() to finish off the scmd.
> Introduce sas_end_task() to capture the final response status from the
> lldd and free the task.
> Also take the opportunity to kill this warning.
> drivers/scsi/libsas/sas_scsi_host.c: In function ‘sas_end_task’:
> drivers/scsi/libsas/sas_scsi_host.c:102:3: warning: case value ‘2’ not in enumerated type ‘enum exec_status’ [-Wswitch]
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
> drivers/scsi/libsas/sas_scsi_host.c | 61 +++++++++++++++++++----------------
> include/scsi/libsas.h | 5 ++-
> 2 files changed, 37 insertions(+), 29 deletions(-)
> diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c
> index f32c628..13aee61 100644
> --- a/drivers/scsi/libsas/sas_scsi_host.c
> +++ b/drivers/scsi/libsas/sas_scsi_host.c
> @@ -49,27 +49,12 @@
> #include<linux/scatterlist.h>
> #include<linux/libata.h>
>
> -/* ---------- SCSI Host glue ---------- */
> -
> -static void sas_scsi_task_done(struct sas_task *task)
> +/* record final status and free the task */
> +static void sas_end_task(struct scsi_cmnd *sc, struct sas_task *task)
> {
> struct task_status_struct *ts =&task->task_status;
> - struct scsi_cmnd *sc = task->uldd_task;
> int hs = 0, stat = 0;
[...]
> @@ -124,10 +109,32 @@ static void sas_scsi_task_done(struct sas_task *task)
> break;
> }
> }
> - ASSIGN_SAS_TASK(sc, NULL);
> +
> sc->result = (hs<< 16) | stat;
> + ASSIGN_SAS_TASK(sc, NULL);
> list_del_init(&task->list);
> sas_free_task(task);
> +}
> +
> +static void sas_scsi_task_done(struct sas_task *task)
> +{
> + struct scsi_cmnd *sc = task->uldd_task;
> +
> + if (unlikely(task->task_state_flags& SAS_TASK_STATE_ABORTED)) {
> + /* Aborted tasks will be completed by the error handler */
> + SAS_DPRINTK("task done but aborted\n");
> + return;
> + }
> +
> + if (unlikely(!sc)) {
> + SAS_DPRINTK("task_done called with non existing SCSI cmnd!\n");
> + list_del_init(&task->list);
> + sas_free_task(task);
> + return;
> + }
> +
> + ASSIGN_SAS_TASK(sc, NULL);
Why do it twice -- once here and then in sas_end_task()?
> + sas_end_task(sc, task);
> sc->scsi_done(sc);
> }
MBR, Sergei
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH 14/24] libsas: prevent double completion of scmds from eh
2011-12-17 13:08 ` Sergei Shtylyov
@ 2011-12-18 19:19 ` Dan Williams
0 siblings, 0 replies; 39+ messages in thread
From: Dan Williams @ 2011-12-18 19:19 UTC (permalink / raw)
To: Sergei Shtylyov; +Cc: linux-scsi, linux-ide
On Sat, Dec 17, 2011 at 5:08 AM, Sergei Shtylyov <sshtylyov@mvista.com> wrote:
>> + ASSIGN_SAS_TASK(sc, NULL);
>
>
> Why do it twice -- once here and then in sas_end_task()?
...for the sas_eh_finish_cmd() case where eh has taken responsibility
for cleanup and the sas_scsi_task_done() path has been disabled.
--
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] 39+ messages in thread
* Re: [PATCH 14/24] libsas: prevent double completion of scmds from eh
2011-12-17 2:34 ` [PATCH 14/24] libsas: prevent double completion of scmds from eh Dan Williams
2011-12-17 13:08 ` Sergei Shtylyov
@ 2011-12-17 13:13 ` Sergei Shtylyov
2011-12-18 19:24 ` Dan Williams
1 sibling, 1 reply; 39+ messages in thread
From: Sergei Shtylyov @ 2011-12-17 13:13 UTC (permalink / raw)
To: Dan Williams; +Cc: linux-scsi, linux-ide
On 17-12-2011 6:34, Dan Williams wrote:
> We invoke task->task_done() to free the task in the eh case, but at this
> point we are prepared for scsi_eh_flush_done_q() to finish off the scmd.
> Introduce sas_end_task() to capture the final response status from the
> lldd and free the task.
> Also take the opportunity to kill this warning.
> drivers/scsi/libsas/sas_scsi_host.c: In function ‘sas_end_task’:
> drivers/scsi/libsas/sas_scsi_host.c:102:3: warning: case value ‘2’ not in enumerated type ‘enum exec_status’ [-Wswitch]
Perhaps this a material for a separate patch...
> Signed-off-by: Dan Williams<dan.j.williams@intel.com>
> ---
> drivers/scsi/libsas/sas_scsi_host.c | 61 +++++++++++++++++++----------------
> include/scsi/libsas.h | 5 ++-
> 2 files changed, 37 insertions(+), 29 deletions(-)
> diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
> index 6064f82..b6b0b99 100644
> --- a/include/scsi/libsas.h
> +++ b/include/scsi/libsas.h
> @@ -443,7 +443,10 @@ enum service_response {
> };
>
> enum exec_status {
> - /* The SAM_STAT_.. codes fit in the lower 6 bits */
> + /* The SAM_STAT_.. codes fit in the lower 6 bits, alias some of
> + * them here to silence 'case value not in enumerated type' warnings
> + */
> + __SAM_STAT_CHECK_CONDITION = SAM_STAT_CHECK_CONDITION,
Looks like you forgot to change the problematic *case* itself...
MBR, Sergei
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH 14/24] libsas: prevent double completion of scmds from eh
2011-12-17 13:13 ` Sergei Shtylyov
@ 2011-12-18 19:24 ` Dan Williams
0 siblings, 0 replies; 39+ messages in thread
From: Dan Williams @ 2011-12-18 19:24 UTC (permalink / raw)
To: Sergei Shtylyov; +Cc: linux-scsi, linux-ide
On Sat, Dec 17, 2011 at 5:13 AM, Sergei Shtylyov <sshtylyov@mvista.com> wrote:
> Perhaps this a material for a separate patch...
If it changed behavior maybe we'd want it as a separate bisection
point, or if this approach for silencing this type of warnings gets
nak'd.
[..]
>> enum exec_status {
>> - /* The SAM_STAT_.. codes fit in the lower 6 bits */
>> + /* The SAM_STAT_.. codes fit in the lower 6 bits, alias some of
>> + * them here to silence 'case value not in enumerated type'
>> warnings
>> + */
>> + __SAM_STAT_CHECK_CONDITION = SAM_STAT_CHECK_CONDITION,
>
>
> Looks like you forgot to change the problematic *case* itself...
It was deliberate. This is just an alias to get the 'value' in the
enum, but I think the real definition should be used in all cases.
--
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] 39+ messages in thread
* [PATCH 15/24] libsas: fix timeout vs completion race
2011-12-17 2:33 [PATCH 00/24] libsas: eh reworks (ata-eh vs discovery, races, ...) Dan Williams
` (13 preceding siblings ...)
2011-12-17 2:34 ` [PATCH 14/24] libsas: prevent double completion of scmds from eh Dan Williams
@ 2011-12-17 2:34 ` Dan Williams
2011-12-17 2:34 ` [PATCH 16/24] libsas: let libata handle command timeouts Dan Williams
` (8 subsequent siblings)
23 siblings, 0 replies; 39+ messages in thread
From: Dan Williams @ 2011-12-17 2:34 UTC (permalink / raw)
To: linux-scsi; +Cc: Tejun Heo, linux-ide, Christoph Hellwig, Darrick J. Wong
Until we have told the lldd to forget a task a timed out operation can
return from the hardware at any time. Since completion frees the task
we need to make sure that no tasks run their normal completion handler
once eh has decided to manage the task. Similar to
ata_scsi_cmd_error_handler() freeze completions to let eh judge the
outcome of the race.
Task collector mode is problematic because it presents a situation where
a task can be timed out and aborted before the lldd has even seen it.
For this case we need to guarantee that a task that an lldd has been
told to forget does not get queued after the lldd says "never seen it".
With sas_scsi_timed_out we achieve this with the ->task_queue_flush
mutex, rather than adding more time.
Cc: Tejun Heo <tj@kernel.org>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Darrick J. Wong <djwong@us.ibm.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/scsi/libsas/sas_ata.c | 35 ++++--------
drivers/scsi/libsas/sas_init.c | 4 +
drivers/scsi/libsas/sas_internal.h | 3 +
drivers/scsi/libsas/sas_scsi_host.c | 104 +++++++++++++++++------------------
include/scsi/libsas.h | 6 +-
include/scsi/sas_ata.h | 8 ---
6 files changed, 72 insertions(+), 88 deletions(-)
diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index c989d7f..bbbf6c9 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -93,21 +93,30 @@ static enum ata_completion_errors sas_to_ata_err(struct task_status_struct *ts)
static void sas_ata_task_done(struct sas_task *task)
{
struct ata_queued_cmd *qc = task->uldd_task;
- struct domain_device *dev;
+ struct domain_device *dev = task->dev;
struct task_status_struct *stat = &task->task_status;
struct ata_task_resp *resp = (struct ata_task_resp *)stat->buf;
- struct sas_ha_struct *sas_ha;
+ struct sas_ha_struct *sas_ha = dev->port->ha;
enum ata_completion_errors ac;
unsigned long flags;
struct ata_link *link;
struct ata_port *ap;
+ spin_lock_irqsave(&dev->done_lock, flags);
+ if (test_bit(SAS_HA_FROZEN, &sas_ha->state))
+ task = NULL;
+ else if (qc && qc->scsicmd)
+ ASSIGN_SAS_TASK(qc->scsicmd, NULL);
+ spin_unlock_irqrestore(&dev->done_lock, flags);
+
+ /* check if libsas-eh got to the task before us */
+ if (unlikely(!task))
+ return;
+
if (!qc)
goto qc_already_gone;
ap = qc->ap;
- dev = ap->private_data;
- sas_ha = dev->port->ha;
link = &ap->link;
spin_lock_irqsave(ap->lock, flags);
@@ -156,8 +165,6 @@ static void sas_ata_task_done(struct sas_task *task)
}
qc->lldd_task = NULL;
- if (qc->scsicmd)
- ASSIGN_SAS_TASK(qc->scsicmd, NULL);
ata_qc_complete(qc);
spin_unlock_irqrestore(ap->lock, flags);
@@ -619,22 +626,6 @@ void sas_ata_strategy_handler(struct Scsi_Host *shost)
mutex_unlock(&sas_ha->eh_mutex);
}
-int sas_ata_timed_out(struct scsi_cmnd *cmd, struct sas_task *task,
- enum blk_eh_timer_return *rtn)
-{
- struct domain_device *ddev = cmd_to_domain_dev(cmd);
-
- if (!dev_is_sata(ddev) || task)
- return 0;
-
- /* we're a sata device with no task, so this must be a libata
- * eh timeout. Ideally should hook into libata timeout
- * handling, but there's no point, it just wants to activate
- * the eh thread */
- *rtn = BLK_EH_NOT_HANDLED;
- return 1;
-}
-
int sas_ata_eh(struct Scsi_Host *shost, struct list_head *work_q,
struct list_head *done_q)
{
diff --git a/drivers/scsi/libsas/sas_init.c b/drivers/scsi/libsas/sas_init.c
index 55e34c6..b04b3ce 100644
--- a/drivers/scsi/libsas/sas_init.c
+++ b/drivers/scsi/libsas/sas_init.c
@@ -113,7 +113,7 @@ int sas_register_ha(struct sas_ha_struct *sas_ha)
else if (sas_ha->lldd_queue_size == -1)
sas_ha->lldd_queue_size = 128; /* Sanity */
- sas_ha->state = SAS_HA_REGISTERED;
+ set_bit(SAS_HA_REGISTERED, &sas_ha->state);
spin_lock_init(&sas_ha->state_lock);
error = sas_register_phys(sas_ha);
@@ -161,7 +161,7 @@ int sas_unregister_ha(struct sas_ha_struct *sas_ha)
/* Set the state to unregistered to avoid further
* events to be queued */
spin_lock_irqsave(&sas_ha->state_lock, flags);
- sas_ha->state = SAS_HA_UNREGISTERED;
+ clear_bit(SAS_HA_REGISTERED, &sas_ha->state);
spin_unlock_irqrestore(&sas_ha->state_lock, flags);
scsi_flush_work(sas_ha->core.shost);
diff --git a/drivers/scsi/libsas/sas_internal.h b/drivers/scsi/libsas/sas_internal.h
index 128c941..141a2df 100644
--- a/drivers/scsi/libsas/sas_internal.h
+++ b/drivers/scsi/libsas/sas_internal.h
@@ -100,7 +100,7 @@ static inline void sas_queue_event(int event, unsigned long *pending,
unsigned long flags;
spin_lock_irqsave(&sas_ha->state_lock, flags);
- if (sas_ha->state != SAS_HA_UNREGISTERED)
+ if (test_bit(SAS_HA_REGISTERED, &sas_ha->state))
scsi_queue_work(sas_ha->core.shost, work);
spin_unlock_irqrestore(&sas_ha->state_lock, flags);
}
@@ -154,6 +154,7 @@ static inline struct domain_device *sas_alloc_device(void)
INIT_LIST_HEAD(&dev->dev_list_node);
INIT_LIST_HEAD(&dev->disco_list_node);
kref_init(&dev->kref);
+ spin_lock_init(&dev->done_lock);
}
return dev;
}
diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c
index 13aee61..3fca198 100644
--- a/drivers/scsi/libsas/sas_scsi_host.c
+++ b/drivers/scsi/libsas/sas_scsi_host.c
@@ -119,9 +119,19 @@ static void sas_end_task(struct scsi_cmnd *sc, struct sas_task *task)
static void sas_scsi_task_done(struct sas_task *task)
{
struct scsi_cmnd *sc = task->uldd_task;
+ struct domain_device *dev = task->dev;
+ struct sas_ha_struct *ha = dev->port->ha;
+ unsigned long flags;
+
+ spin_lock_irqsave(&dev->done_lock, flags);
+ if (test_bit(SAS_HA_FROZEN, &ha->state))
+ task = NULL;
+ else
+ ASSIGN_SAS_TASK(sc, NULL);
+ spin_unlock_irqrestore(&dev->done_lock, flags);
- if (unlikely(task->task_state_flags & SAS_TASK_STATE_ABORTED)) {
- /* Aborted tasks will be completed by the error handler */
+ if (unlikely(!task)) {
+ /* task will be completed by the error handler */
SAS_DPRINTK("task done but aborted\n");
return;
}
@@ -133,7 +143,6 @@ static void sas_scsi_task_done(struct sas_task *task)
return;
}
- ASSIGN_SAS_TASK(sc, NULL);
sas_end_task(sc, task);
sc->scsi_done(sc);
}
@@ -298,6 +307,7 @@ enum task_disposition {
TASK_IS_DONE,
TASK_IS_ABORTED,
TASK_IS_AT_LU,
+ TASK_IS_NOT_AT_HA,
TASK_IS_NOT_AT_LU,
TASK_ABORT_FAILED,
};
@@ -314,19 +324,18 @@ static enum task_disposition sas_scsi_find_task(struct sas_task *task)
struct scsi_core *core = &ha->core;
struct sas_task *t, *n;
+ mutex_lock(&core->task_queue_flush);
spin_lock_irqsave(&core->task_queue_lock, flags);
- list_for_each_entry_safe(t, n, &core->task_queue, list) {
+ list_for_each_entry_safe(t, n, &core->task_queue, list)
if (task == t) {
list_del_init(&t->list);
- spin_unlock_irqrestore(&core->task_queue_lock,
- flags);
- SAS_DPRINTK("%s: task 0x%p aborted from "
- "task_queue\n",
- __func__, task);
- return TASK_IS_ABORTED;
+ break;
}
- }
spin_unlock_irqrestore(&core->task_queue_lock, flags);
+ mutex_unlock(&core->task_queue_flush);
+
+ if (task == t)
+ return TASK_IS_NOT_AT_HA;
}
for (i = 0; i < 5; i++) {
@@ -499,8 +508,7 @@ try_bus_reset:
}
static int sas_eh_handle_sas_errors(struct Scsi_Host *shost,
- struct list_head *work_q,
- struct list_head *done_q)
+ struct list_head *work_q)
{
struct scsi_cmnd *cmd, *n;
enum task_disposition res = TASK_IS_DONE;
@@ -511,7 +519,16 @@ static int sas_eh_handle_sas_errors(struct Scsi_Host *shost,
Again:
list_for_each_entry_safe(cmd, n, work_q, eh_entry) {
- struct sas_task *task = TO_SAS_TASK(cmd);
+ struct domain_device *dev = cmd_to_domain_dev(cmd);
+ struct sas_task *task;
+
+ spin_lock_irqsave(&dev->done_lock, flags);
+ /* by this point the lldd has either observed
+ * SAS_HA_FROZEN and is leaving the task alone, or has
+ * won the race with eh and decided to complete it
+ */
+ task = TO_SAS_TASK(cmd);
+ spin_unlock_irqrestore(&dev->done_lock, flags);
if (!task)
continue;
@@ -534,6 +551,14 @@ Again:
cmd->eh_eflags = 0;
switch (res) {
+ case TASK_IS_NOT_AT_HA:
+ SAS_DPRINTK("%s: task 0x%p is not at ha: %s\n",
+ __func__, task,
+ cmd->retries ? "retry" : "aborted");
+ if (cmd->retries)
+ cmd->retries--;
+ sas_eh_finish_cmd(cmd);
+ continue;
case TASK_IS_DONE:
SAS_DPRINTK("%s: task 0x%p is done\n", __func__,
task);
@@ -635,7 +660,8 @@ void sas_scsi_recover_host(struct Scsi_Host *shost)
* Deal with commands that still have SAS tasks (i.e. they didn't
* complete via the normal sas_task completion mechanism)
*/
- if (sas_eh_handle_sas_errors(shost, &eh_work_q, &ha->eh_done_q))
+ set_bit(SAS_HA_FROZEN, &ha->state);
+ if (sas_eh_handle_sas_errors(shost, &eh_work_q))
goto out;
/*
@@ -649,6 +675,10 @@ void sas_scsi_recover_host(struct Scsi_Host *shost)
scsi_eh_ready_devs(shost, &eh_work_q, &ha->eh_done_q);
out:
+ clear_bit(SAS_HA_FROZEN, &ha->state);
+ if (ha->lldd_max_execute_num > 1)
+ wake_up_process(ha->core.queue_thread);
+
/* now link into libata eh --- if we have any ata devices */
sas_ata_strategy_handler(shost);
@@ -660,43 +690,7 @@ out:
enum blk_eh_timer_return sas_scsi_timed_out(struct scsi_cmnd *cmd)
{
- struct sas_task *task = TO_SAS_TASK(cmd);
- unsigned long flags;
- enum blk_eh_timer_return rtn;
-
- if (sas_ata_timed_out(cmd, task, &rtn))
- return rtn;
-
- if (!task) {
- cmd->request->timeout /= 2;
- SAS_DPRINTK("command 0x%p, task 0x%p, gone: %s\n",
- cmd, task, (cmd->request->timeout ?
- "BLK_EH_RESET_TIMER" : "BLK_EH_NOT_HANDLED"));
- if (!cmd->request->timeout)
- return BLK_EH_NOT_HANDLED;
- return BLK_EH_RESET_TIMER;
- }
-
- spin_lock_irqsave(&task->task_state_lock, flags);
- BUG_ON(task->task_state_flags & SAS_TASK_STATE_ABORTED);
- if (task->task_state_flags & SAS_TASK_STATE_DONE) {
- spin_unlock_irqrestore(&task->task_state_lock, flags);
- SAS_DPRINTK("command 0x%p, task 0x%p, timed out: "
- "BLK_EH_HANDLED\n", cmd, task);
- return BLK_EH_HANDLED;
- }
- if (!(task->task_state_flags & SAS_TASK_AT_INITIATOR)) {
- spin_unlock_irqrestore(&task->task_state_lock, flags);
- SAS_DPRINTK("command 0x%p, task 0x%p, not at initiator: "
- "BLK_EH_RESET_TIMER\n",
- cmd, task);
- return BLK_EH_RESET_TIMER;
- }
- task->task_state_flags |= SAS_TASK_STATE_ABORTED;
- spin_unlock_irqrestore(&task->task_state_lock, flags);
-
- SAS_DPRINTK("command 0x%p, task 0x%p, timed out: BLK_EH_NOT_HANDLED\n",
- cmd, task);
+ scmd_printk(KERN_DEBUG, cmd, "command %p timed out\n", cmd);
return BLK_EH_NOT_HANDLED;
}
@@ -867,9 +861,11 @@ static void sas_queue(struct sas_ha_struct *sas_ha)
int res;
struct sas_internal *i = to_sas_internal(core->shost->transportt);
+ mutex_lock(&core->task_queue_flush);
spin_lock_irqsave(&core->task_queue_lock, flags);
while (!kthread_should_stop() &&
- !list_empty(&core->task_queue)) {
+ !list_empty(&core->task_queue) &&
+ !test_bit(SAS_HA_FROZEN, &sas_ha->state)) {
can_queue = sas_ha->lldd_queue_size - core->task_queue_size;
if (can_queue >= 0) {
@@ -905,6 +901,7 @@ static void sas_queue(struct sas_ha_struct *sas_ha)
}
}
spin_unlock_irqrestore(&core->task_queue_lock, flags);
+ mutex_unlock(&core->task_queue_flush);
}
/**
@@ -931,6 +928,7 @@ int sas_init_queue(struct sas_ha_struct *sas_ha)
struct scsi_core *core = &sas_ha->core;
spin_lock_init(&core->task_queue_lock);
+ mutex_init(&core->task_queue_flush);
core->task_queue_size = 0;
INIT_LIST_HEAD(&core->task_queue);
diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index b6b0b99..ccddfae 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -173,6 +173,7 @@ struct sata_device {
/* ---------- Domain device ---------- */
struct domain_device {
+ spinlock_t done_lock;
enum sas_dev_type dev_type;
enum sas_linkrate linkrate;
@@ -315,6 +316,7 @@ struct asd_sas_phy {
struct scsi_core {
struct Scsi_Host *shost;
+ struct mutex task_queue_flush;
spinlock_t task_queue_lock;
struct list_head task_queue;
int task_queue_size;
@@ -329,7 +331,7 @@ struct sas_ha_event {
enum sas_ha_state {
SAS_HA_REGISTERED,
- SAS_HA_UNREGISTERED
+ SAS_HA_FROZEN
};
struct sas_ha_struct {
@@ -337,7 +339,7 @@ struct sas_ha_struct {
struct sas_ha_event ha_events[HA_NUM_EVENTS];
unsigned long pending;
- enum sas_ha_state state;
+ unsigned long state;
spinlock_t state_lock;
struct mutex eh_mutex;
diff --git a/include/scsi/sas_ata.h b/include/scsi/sas_ata.h
index 557fc9a..9f7a23d 100644
--- a/include/scsi/sas_ata.h
+++ b/include/scsi/sas_ata.h
@@ -41,8 +41,6 @@ int sas_ata_init_host_and_port(struct domain_device *found_dev,
void sas_ata_task_abort(struct sas_task *task);
void sas_ata_strategy_handler(struct Scsi_Host *shost);
-int sas_ata_timed_out(struct scsi_cmnd *cmd, struct sas_task *task,
- enum blk_eh_timer_return *rtn);
int sas_ata_eh(struct Scsi_Host *shost, struct list_head *work_q,
struct list_head *done_q);
void sas_probe_sata(struct work_struct *work);
@@ -67,12 +65,6 @@ static inline void sas_ata_strategy_handler(struct Scsi_Host *shost)
{
}
-static inline int sas_ata_timed_out(struct scsi_cmnd *cmd,
- struct sas_task *task,
- enum blk_eh_timer_return *rtn)
-{
- return 0;
-}
static inline int sas_ata_eh(struct Scsi_Host *shost, struct list_head *work_q,
struct list_head *done_q)
{
^ permalink raw reply related [flat|nested] 39+ messages in thread* [PATCH 16/24] libsas: let libata handle command timeouts
2011-12-17 2:33 [PATCH 00/24] libsas: eh reworks (ata-eh vs discovery, races, ...) Dan Williams
` (14 preceding siblings ...)
2011-12-17 2:34 ` [PATCH 15/24] libsas: fix timeout vs completion race Dan Williams
@ 2011-12-17 2:34 ` Dan Williams
2011-12-17 2:34 ` [PATCH 17/24] libsas: defer SAS_TASK_NEED_DEV_RESET commands to libata Dan Williams
` (7 subsequent siblings)
23 siblings, 0 replies; 39+ messages in thread
From: Dan Williams @ 2011-12-17 2:34 UTC (permalink / raw)
To: linux-scsi; +Cc: linux-ide, Andrzej Jakowski
libsas-eh if it successfully aborts an ata command will hide the timeout
condition (AC_ERR_TIMEOUT) from libata. The command likely completes
with the all-zero task->task_status it started with. Instead, interpret
a TMF_RESP_FUNC_COMPLETE as the end of the sas_task but keep the scmd
around for libata-eh to handle.
Tested-by: Andrzej Jakowski <andrzej.jakowski@intel.com>
signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/scsi/libsas/sas_init.c | 1 +
drivers/scsi/libsas/sas_scsi_host.c | 22 ++++++++++++++++++++--
include/scsi/libsas.h | 3 ++-
3 files changed, 23 insertions(+), 3 deletions(-)
diff --git a/drivers/scsi/libsas/sas_init.c b/drivers/scsi/libsas/sas_init.c
index b04b3ce..934c9e7 100644
--- a/drivers/scsi/libsas/sas_init.c
+++ b/drivers/scsi/libsas/sas_init.c
@@ -144,6 +144,7 @@ int sas_register_ha(struct sas_ha_struct *sas_ha)
}
INIT_LIST_HEAD(&sas_ha->eh_done_q);
+ INIT_LIST_HEAD(&sas_ha->eh_ata_q);
return 0;
diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c
index 3fca198..aa1fe72 100644
--- a/drivers/scsi/libsas/sas_scsi_host.c
+++ b/drivers/scsi/libsas/sas_scsi_host.c
@@ -265,6 +265,22 @@ static void sas_eh_finish_cmd(struct scsi_cmnd *cmd)
scsi_eh_finish_cmd(cmd, &sas_ha->eh_done_q);
}
+static void sas_eh_defer_cmd(struct scsi_cmnd *cmd)
+{
+ struct sas_task *task = TO_SAS_TASK(cmd);
+ struct domain_device *dev = task->dev;
+ struct sas_ha_struct *ha = dev->port->ha;
+
+ if (!dev_is_sata(dev)) {
+ sas_eh_finish_cmd(cmd);
+ return;
+ }
+
+ /* report the timeout to libata */
+ sas_end_task(cmd, task);
+ list_move_tail(&cmd->eh_entry, &ha->eh_ata_q);
+}
+
static void sas_scsi_clear_queue_lu(struct list_head *error_q, struct scsi_cmnd *my_cmd)
{
struct scsi_cmnd *cmd, *n;
@@ -562,12 +578,12 @@ Again:
case TASK_IS_DONE:
SAS_DPRINTK("%s: task 0x%p is done\n", __func__,
task);
- sas_eh_finish_cmd(cmd);
+ sas_eh_defer_cmd(cmd);
continue;
case TASK_IS_ABORTED:
SAS_DPRINTK("%s: task 0x%p is aborted\n",
__func__, task);
- sas_eh_finish_cmd(cmd);
+ sas_eh_defer_cmd(cmd);
continue;
case TASK_IS_AT_LU:
SAS_DPRINTK("task 0x%p is at LU: lu recover\n", task);
@@ -635,12 +651,14 @@ Again:
goto clear_q;
}
}
+ list_splice_tail_init(&ha->eh_ata_q, work_q);
return list_empty(work_q);
clear_q:
SAS_DPRINTK("--- Exit %s -- clear_q\n", __func__);
list_for_each_entry_safe(cmd, n, work_q, eh_entry)
sas_eh_finish_cmd(cmd);
+ list_splice_tail_init(&ha->eh_ata_q, work_q);
return list_empty(work_q);
}
diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index ccddfae..3ffc605 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -372,7 +372,8 @@ struct sas_ha_struct {
void *lldd_ha; /* not touched by sas class code */
- struct list_head eh_done_q;
+ struct list_head eh_done_q; /* complete via scsi_eh_flush_done_q */
+ struct list_head eh_ata_q; /* scmds to promote from sas to ata eh */
};
#define SHOST_TO_SAS_HA(_shost) (*(struct sas_ha_struct **)(_shost)->hostdata)
^ permalink raw reply related [flat|nested] 39+ messages in thread* [PATCH 17/24] libsas: defer SAS_TASK_NEED_DEV_RESET commands to libata
2011-12-17 2:33 [PATCH 00/24] libsas: eh reworks (ata-eh vs discovery, races, ...) Dan Williams
` (15 preceding siblings ...)
2011-12-17 2:34 ` [PATCH 16/24] libsas: let libata handle command timeouts Dan Williams
@ 2011-12-17 2:34 ` Dan Williams
2011-12-17 2:34 ` [PATCH 18/24] libsas: use libata-eh-reset for sata rediscovery fis transmit failures Dan Williams
` (6 subsequent siblings)
23 siblings, 0 replies; 39+ messages in thread
From: Dan Williams @ 2011-12-17 2:34 UTC (permalink / raw)
To: linux-scsi; +Cc: linux-ide, Darrick J. Wong
lldds use the SAS_TASK_NEED_DEV_RESET interface to request that eh
perform a reset. In the sata device case defer the commands that
triggered the reset to libata-eh context so it can perform its pre and
post reset management.
In the sas_ata_post_internal() case the reset request is falling a deaf
ears as the sas_task is immediately destroyed without any reset action.
Since it is currently a nop, and likely superfluous given the conversion
to new-style libata-eh, just drop the request.
Cc: Darrick J. Wong <djwong@us.ibm.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/scsi/libsas/sas_ata.c | 14 ++++----------
drivers/scsi/libsas/sas_scsi_host.c | 4 ++--
2 files changed, 6 insertions(+), 12 deletions(-)
diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index bbbf6c9..bacd3ba 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -407,18 +407,12 @@ static void sas_ata_post_internal(struct ata_queued_cmd *qc)
* ourselves.
*/
struct sas_task *task = qc->lldd_task;
- unsigned long flags;
qc->lldd_task = NULL;
- if (task) {
- /* Should this be a AT(API) device reset? */
- spin_lock_irqsave(&task->task_state_lock, flags);
- task->task_state_flags |= SAS_TASK_NEED_DEV_RESET;
- spin_unlock_irqrestore(&task->task_state_lock, flags);
-
- task->uldd_task = NULL;
- sas_ata_internal_abort(task);
- }
+ if (!task)
+ return;
+ task->uldd_task = NULL;
+ sas_ata_internal_abort(task);
}
}
diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c
index aa1fe72..e51a909 100644
--- a/drivers/scsi/libsas/sas_scsi_host.c
+++ b/drivers/scsi/libsas/sas_scsi_host.c
@@ -288,7 +288,7 @@ static void sas_scsi_clear_queue_lu(struct list_head *error_q, struct scsi_cmnd
list_for_each_entry_safe(cmd, n, error_q, eh_entry) {
if (cmd->device->sdev_target == my_cmd->device->sdev_target &&
cmd->device->lun == my_cmd->device->lun)
- sas_eh_finish_cmd(cmd);
+ sas_eh_defer_cmd(cmd);
}
}
@@ -594,7 +594,7 @@ Again:
"recovered\n",
SAS_ADDR(task->dev),
cmd->device->lun);
- sas_eh_finish_cmd(cmd);
+ sas_eh_defer_cmd(cmd);
sas_scsi_clear_queue_lu(work_q, cmd);
goto Again;
}
^ permalink raw reply related [flat|nested] 39+ messages in thread* [PATCH 18/24] libsas: use libata-eh-reset for sata rediscovery fis transmit failures
2011-12-17 2:33 [PATCH 00/24] libsas: eh reworks (ata-eh vs discovery, races, ...) Dan Williams
` (16 preceding siblings ...)
2011-12-17 2:34 ` [PATCH 17/24] libsas: defer SAS_TASK_NEED_DEV_RESET commands to libata Dan Williams
@ 2011-12-17 2:34 ` Dan Williams
2011-12-17 2:34 ` [PATCH 19/24] libsas: execute transport link resets with libata-eh via host workqueue Dan Williams
` (5 subsequent siblings)
23 siblings, 0 replies; 39+ messages in thread
From: Dan Williams @ 2011-12-17 2:34 UTC (permalink / raw)
To: linux-scsi; +Cc: linux-ide
Since sata devices can take several seconds to recover the link on reset
the 0.5 seconds that libsas currently waits may not be enough. Instead
if we are rediscovering a phy that was previously attached to a sata
device let libata handle any resets to encourage the device to transmit
the initial fis.
Once sas_ata_hard_reset() and lldds learn how to honor 'deadline' libsas
should stop encountering phys in an intermediate state, until then this
will loop until the fis is transmitted or ->attached_sas_addr gets
cleared, but in the more likely initial discovery case we keep existing
behavior.
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/scsi/libsas/sas_ata.c | 20 ++++++++++++++++
drivers/scsi/libsas/sas_expander.c | 44 ++++++++++++++++++++++++++++++++----
include/scsi/sas_ata.h | 6 ++++-
3 files changed, 64 insertions(+), 6 deletions(-)
diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index bacd3ba..94354d5 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -665,3 +665,23 @@ int sas_ata_eh(struct Scsi_Host *shost, struct list_head *work_q,
return rtn;
}
+
+void sas_ata_schedule_reset(struct domain_device *dev)
+{
+ struct ata_eh_info *ehi;
+ struct ata_port *ap;
+ unsigned long flags;
+
+ if (!dev_is_sata(dev))
+ return;
+
+ ap = dev->sata_dev.ap;
+ ehi = &ap->link.eh_info;
+
+ spin_lock_irqsave(ap->lock, flags);
+ ehi->err_mask |= AC_ERR_TIMEOUT;
+ ehi->action |= ATA_EH_RESET;
+ spin_unlock_irqrestore(ap->lock, flags);
+
+ ata_port_schedule_eh(ap);
+}
diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index c3846cf..9d2bb32 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -28,6 +28,7 @@
#include "sas_internal.h"
+#include <scsi/sas_ata.h>
#include <scsi/scsi_transport.h>
#include <scsi/scsi_transport_sas.h>
#include "../scsi_sas_internal.h"
@@ -226,12 +227,35 @@ static void sas_set_ex_phy(struct domain_device *dev, int phy_id,
return;
}
+/* check if we have an existing attached ata device on this expander phy */
+struct domain_device *sas_ex_to_ata(struct domain_device *ex_dev, int phy_id)
+{
+ struct ex_phy *ex_phy = &ex_dev->ex_dev.ex_phy[phy_id];
+ struct domain_device *dev;
+ struct sas_rphy *rphy;
+
+ if (!ex_phy->port)
+ return NULL;
+
+ rphy = ex_phy->port->rphy;
+ if (!rphy)
+ return NULL;
+
+ dev = sas_find_dev_by_rphy(rphy);
+
+ if (dev && dev_is_sata(dev))
+ return dev;
+
+ return NULL;
+}
+
#define DISCOVER_REQ_SIZE 16
#define DISCOVER_RESP_SIZE 56
static int sas_ex_phy_discover_helper(struct domain_device *dev, u8 *disc_req,
u8 *disc_resp, int single)
{
+ struct domain_device *ata_dev = sas_ex_to_ata(dev, single);
int i, res;
disc_req[9] = single;
@@ -242,20 +266,30 @@ static int sas_ex_phy_discover_helper(struct domain_device *dev, u8 *disc_req,
disc_resp, DISCOVER_RESP_SIZE);
if (res)
return res;
- /* This is detecting a failure to transmit initial
- * dev to host FIS as described in section G.5 of
- * sas-2 r 04b */
dr = &((struct smp_resp *)disc_resp)->disc;
if (memcmp(dev->sas_addr, dr->attached_sas_addr,
SAS_ADDR_SIZE) == 0) {
sas_printk("Found loopback topology, just ignore it!\n");
return 0;
}
+
+ /* This is detecting a failure to transmit initial
+ * dev to host FIS as described in section J.5 of
+ * sas-2 r16
+ */
if (!(dr->attached_dev_type == 0 &&
dr->attached_sata_dev))
break;
- /* In order to generate the dev to host FIS, we
- * send a link reset to the expander port */
+
+ /* In order to generate the dev to host FIS, we send a
+ * link reset to the expander port. If a device was
+ * previously detected on this port we ask libata to
+ * manage the reset and link recovery.
+ */
+ if (ata_dev) {
+ sas_ata_schedule_reset(ata_dev);
+ break;
+ }
sas_smp_phy_control(dev, single, PHY_FUNC_LINK_RESET, NULL);
/* Wait for the reset to trigger the negotiation */
msleep(500);
diff --git a/include/scsi/sas_ata.h b/include/scsi/sas_ata.h
index 9f7a23d..c0bcd30 100644
--- a/include/scsi/sas_ata.h
+++ b/include/scsi/sas_ata.h
@@ -44,7 +44,7 @@ void sas_ata_strategy_handler(struct Scsi_Host *shost);
int sas_ata_eh(struct Scsi_Host *shost, struct list_head *work_q,
struct list_head *done_q);
void sas_probe_sata(struct work_struct *work);
-
+void sas_ata_schedule_reset(struct domain_device *dev);
#else
@@ -75,6 +75,10 @@ static inline void sas_probe_sata(struct work_struct *work)
{
}
+static inline void sas_ata_schedule_reset(struct domain_device *dev)
+{
+}
+
#endif
#endif /* _SAS_ATA_H_ */
^ permalink raw reply related [flat|nested] 39+ messages in thread* [PATCH 19/24] libsas: execute transport link resets with libata-eh via host workqueue
2011-12-17 2:33 [PATCH 00/24] libsas: eh reworks (ata-eh vs discovery, races, ...) Dan Williams
` (17 preceding siblings ...)
2011-12-17 2:34 ` [PATCH 18/24] libsas: use libata-eh-reset for sata rediscovery fis transmit failures Dan Williams
@ 2011-12-17 2:34 ` Dan Williams
2011-12-17 2:34 ` [PATCH 20/24] libsas: sas_phy_enable via transport_sas_phy_reset Dan Williams
` (4 subsequent siblings)
23 siblings, 0 replies; 39+ messages in thread
From: Dan Williams @ 2011-12-17 2:34 UTC (permalink / raw)
To: linux-scsi; +Cc: linux-ide
Link resets leave ata affiliations intact, so arrange for libsas to make
an effort to avoid dropping the device due to a slow-to-recover link.
Towards this end carry out reset in the host workqueue so that it can
check for ata devices and kick the reset request to libata. Hard
resets, in contrast, bypass libata since they are meant for associating
an ata device with another initiator in the domain (tears down
affiliations).
Need to add a new transport_sas_phy_reset() since the current
sas_phy_reset() is a utility function to libsas lldds. They are not
prepared for it to loop back into eh.
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/ata/libata-eh.c | 1 +
drivers/ata/libata.h | 1 -
drivers/scsi/libsas/sas_ata.c | 11 +++++++
drivers/scsi/libsas/sas_init.c | 56 +++++++++++++++++++++++++++++++++++-
drivers/scsi/libsas/sas_internal.h | 1 +
drivers/scsi/scsi_transport_sas.c | 23 ++++++++++++---
include/linux/libata.h | 1 +
include/scsi/sas_ata.h | 4 +++
include/scsi/scsi_transport_sas.h | 2 +
9 files changed, 93 insertions(+), 7 deletions(-)
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index a9b2820..c61316e 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -863,6 +863,7 @@ void ata_port_wait_eh(struct ata_port *ap)
goto retry;
}
}
+EXPORT_SYMBOL_GPL(ata_port_wait_eh);
static int ata_eh_nr_in_flight(struct ata_port *ap)
{
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index 773de97..78c356d 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -150,7 +150,6 @@ extern void ata_eh_acquire(struct ata_port *ap);
extern void ata_eh_release(struct ata_port *ap);
extern enum blk_eh_timer_return ata_scsi_timed_out(struct scsi_cmnd *cmd);
extern void ata_scsi_error(struct Scsi_Host *host);
-extern void ata_port_wait_eh(struct ata_port *ap);
extern void ata_eh_fastdrain_timerfn(unsigned long arg);
extern void ata_qc_schedule_eh(struct ata_queued_cmd *qc);
extern void ata_dev_disable(struct ata_device *dev);
diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index 94354d5..1df6ed2 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -685,3 +685,14 @@ void sas_ata_schedule_reset(struct domain_device *dev)
ata_port_schedule_eh(ap);
}
+
+void sas_ata_wait_eh(struct domain_device *dev)
+{
+ struct ata_port *ap;
+
+ if (!dev_is_sata(dev))
+ return;
+
+ ap = dev->sata_dev.ap;
+ ata_port_wait_eh(ap);
+}
diff --git a/drivers/scsi/libsas/sas_init.c b/drivers/scsi/libsas/sas_init.c
index 934c9e7..2b8c09e 100644
--- a/drivers/scsi/libsas/sas_init.c
+++ b/drivers/scsi/libsas/sas_init.c
@@ -28,6 +28,7 @@
#include <linux/init.h>
#include <linux/device.h>
#include <linux/spinlock.h>
+#include <scsi/sas_ata.h>
#include <scsi/scsi_host.h>
#include <scsi/scsi_device.h>
#include <scsi/scsi_transport.h>
@@ -191,6 +192,59 @@ static int sas_get_linkerrors(struct sas_phy *phy)
return sas_smp_get_phy_events(phy);
}
+/**
+ * transport_sas_phy_reset - reset a phy and permit libata to manage the link
+ *
+ * phy reset request via sysfs in host workqueue context so we know we
+ * can block on eh and safely traverse the domain_device topology
+ */
+static int transport_sas_phy_reset(struct sas_phy *phy, int hard_reset)
+{
+ int ret;
+ enum phy_func reset_type;
+
+ if (hard_reset)
+ reset_type = PHY_FUNC_HARD_RESET;
+ else
+ reset_type = PHY_FUNC_LINK_RESET;
+
+ if (scsi_is_sas_phy_local(phy)) {
+ struct Scsi_Host *shost = dev_to_shost(phy->dev.parent);
+ struct sas_ha_struct *sas_ha = SHOST_TO_SAS_HA(shost);
+ struct asd_sas_phy *asd_phy = sas_ha->sas_phy[phy->number];
+ struct sas_internal *i =
+ to_sas_internal(sas_ha->core.shost->transportt);
+ struct domain_device *dev = NULL;
+
+ if (asd_phy->port)
+ dev = asd_phy->port->port_dev;
+
+ /* validate that dev has been probed */
+ if (dev)
+ dev = sas_find_dev_by_rphy(dev->rphy);
+
+ if (dev && dev_is_sata(dev) && !hard_reset) {
+ sas_ata_schedule_reset(dev);
+ sas_ata_wait_eh(dev);
+ ret = 0;
+ } else
+ ret = i->dft->lldd_control_phy(asd_phy, reset_type, NULL);
+ } else {
+ struct sas_rphy *rphy = dev_to_rphy(phy->dev.parent);
+ struct domain_device *ddev = sas_find_dev_by_rphy(rphy);
+ struct domain_device *ata_dev = sas_ex_to_ata(ddev, phy->number);
+
+ if (ata_dev && !hard_reset) {
+ sas_ata_schedule_reset(ata_dev);
+ sas_ata_wait_eh(ata_dev);
+ ret = 0;
+ } else
+ ret = sas_smp_phy_control(ddev, phy->number, reset_type, NULL);
+ }
+
+ return ret;
+}
+
int sas_phy_enable(struct sas_phy *phy, int enable)
{
int ret;
@@ -288,7 +342,7 @@ int sas_set_phy_speed(struct sas_phy *phy,
static struct sas_function_template sft = {
.phy_enable = sas_phy_enable,
- .phy_reset = sas_phy_reset,
+ .phy_reset = transport_sas_phy_reset,
.set_phy_speed = sas_set_phy_speed,
.get_linkerrors = sas_get_linkerrors,
.smp_handler = sas_smp_handler,
diff --git a/drivers/scsi/libsas/sas_internal.h b/drivers/scsi/libsas/sas_internal.h
index 141a2df..5879e72 100644
--- a/drivers/scsi/libsas/sas_internal.h
+++ b/drivers/scsi/libsas/sas_internal.h
@@ -73,6 +73,7 @@ int sas_smp_phy_control(struct domain_device *dev, int phy_id,
int sas_smp_get_phy_events(struct sas_phy *phy);
struct domain_device *sas_find_dev_by_rphy(struct sas_rphy *rphy);
+struct domain_device *sas_ex_to_ata(struct domain_device *ex_dev, int phy_id);
void sas_hae_reset(struct work_struct *work);
diff --git a/drivers/scsi/scsi_transport_sas.c b/drivers/scsi/scsi_transport_sas.c
index 9d9330a..760b80b 100644
--- a/drivers/scsi/scsi_transport_sas.c
+++ b/drivers/scsi/scsi_transport_sas.c
@@ -609,12 +609,15 @@ do_sas_phy_reset(struct device *dev, size_t count, int hard_reset)
{
struct sas_phy *phy = transport_class_to_phy(dev);
struct Scsi_Host *shost = dev_to_shost(phy->dev.parent);
- struct sas_internal *i = to_sas_internal(shost->transportt);
- int error;
- error = i->f->phy_reset(phy, hard_reset);
- if (error)
- return error;
+ phy->hard_reset = hard_reset;
+ phy->reset_result = 0;
+
+ scsi_queue_work(shost, &phy->reset_work);
+ scsi_flush_work(shost);
+
+ if (phy->reset_result)
+ return phy->reset_result;
return count;
};
@@ -683,6 +686,15 @@ static void sas_phy_release(struct device *dev)
kfree(phy);
}
+static void sas_phy_reset_work(struct work_struct *work)
+{
+ struct sas_phy *phy = container_of(work, typeof(*phy), reset_work);
+ struct Scsi_Host *shost = dev_to_shost(phy->dev.parent);
+ struct sas_internal *i = to_sas_internal(shost->transportt);
+
+ phy->reset_result = i->f->phy_reset(phy, phy->hard_reset);
+}
+
/**
* sas_phy_alloc - allocates and initialize a SAS PHY structure
* @parent: Parent device
@@ -711,6 +723,7 @@ struct sas_phy *sas_phy_alloc(struct device *parent, int number)
phy->dev.parent = get_device(parent);
phy->dev.release = sas_phy_release;
INIT_LIST_HEAD(&phy->port_siblings);
+ INIT_WORK(&phy->reset_work, sas_phy_reset_work);
if (scsi_is_sas_expander_device(parent)) {
struct sas_rphy *rphy = dev_to_rphy(parent);
dev_set_name(&phy->dev, "phy-%d:%d:%d", shost->host_no,
diff --git a/include/linux/libata.h b/include/linux/libata.h
index cafc09a..aa42704 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1147,6 +1147,7 @@ static inline int ata_acpi_cbl_80wire(struct ata_port *ap,
* EH - drivers/ata/libata-eh.c
*/
extern void ata_port_schedule_eh(struct ata_port *ap);
+extern void ata_port_wait_eh(struct ata_port *ap);
extern int ata_link_abort(struct ata_link *link);
extern int ata_port_abort(struct ata_port *ap);
extern int ata_port_freeze(struct ata_port *ap);
diff --git a/include/scsi/sas_ata.h b/include/scsi/sas_ata.h
index c0bcd30..da3f377 100644
--- a/include/scsi/sas_ata.h
+++ b/include/scsi/sas_ata.h
@@ -45,6 +45,7 @@ int sas_ata_eh(struct Scsi_Host *shost, struct list_head *work_q,
struct list_head *done_q);
void sas_probe_sata(struct work_struct *work);
void sas_ata_schedule_reset(struct domain_device *dev);
+void sas_ata_wait_eh(struct domain_device *dev);
#else
@@ -79,6 +80,9 @@ static inline void sas_ata_schedule_reset(struct domain_device *dev)
{
}
+static inline void sas_ata_wait_eh(struct domain_device *dev)
+{
+}
#endif
#endif /* _SAS_ATA_H_ */
diff --git a/include/scsi/scsi_transport_sas.h b/include/scsi/scsi_transport_sas.h
index ffeebc3..c7eea0d 100644
--- a/include/scsi/scsi_transport_sas.h
+++ b/include/scsi/scsi_transport_sas.h
@@ -75,6 +75,8 @@ struct sas_phy {
/* for the list of phys belonging to a port */
struct list_head port_siblings;
+ int hard_reset;
+ int reset_result;
struct work_struct reset_work;
};
^ permalink raw reply related [flat|nested] 39+ messages in thread* [PATCH 20/24] libsas: sas_phy_enable via transport_sas_phy_reset
2011-12-17 2:33 [PATCH 00/24] libsas: eh reworks (ata-eh vs discovery, races, ...) Dan Williams
` (18 preceding siblings ...)
2011-12-17 2:34 ` [PATCH 19/24] libsas: execute transport link resets with libata-eh via host workqueue Dan Williams
@ 2011-12-17 2:34 ` Dan Williams
2011-12-17 2:34 ` [PATCH 21/24] libsas: Remove redundant phy state notification calls Dan Williams
` (3 subsequent siblings)
23 siblings, 0 replies; 39+ messages in thread
From: Dan Williams @ 2011-12-17 2:34 UTC (permalink / raw)
To: linux-scsi; +Cc: linux-ide
Execute the link-reset triggered by sas_phy_enable via
transport_sas_phy_reset so that it can be managed by libata.
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/scsi/libsas/sas_init.c | 20 +++++++++++++-------
drivers/scsi/libsas/sas_scsi_host.c | 1 -
drivers/scsi/scsi_transport_sas.c | 26 ++++++++++++++++++--------
include/scsi/libsas.h | 1 -
include/scsi/scsi_transport_sas.h | 3 +++
5 files changed, 34 insertions(+), 17 deletions(-)
diff --git a/drivers/scsi/libsas/sas_init.c b/drivers/scsi/libsas/sas_init.c
index 2b8c09e..e461b98 100644
--- a/drivers/scsi/libsas/sas_init.c
+++ b/drivers/scsi/libsas/sas_init.c
@@ -245,15 +245,15 @@ static int transport_sas_phy_reset(struct sas_phy *phy, int hard_reset)
return ret;
}
-int sas_phy_enable(struct sas_phy *phy, int enable)
+static int sas_phy_enable(struct sas_phy *phy, int enable)
{
int ret;
- enum phy_func command;
+ enum phy_func cmd;
if (enable)
- command = PHY_FUNC_LINK_RESET;
+ cmd = PHY_FUNC_LINK_RESET;
else
- command = PHY_FUNC_DISABLE;
+ cmd = PHY_FUNC_DISABLE;
if (scsi_is_sas_phy_local(phy)) {
struct Scsi_Host *shost = dev_to_shost(phy->dev.parent);
@@ -262,15 +262,21 @@ int sas_phy_enable(struct sas_phy *phy, int enable)
struct sas_internal *i =
to_sas_internal(sas_ha->core.shost->transportt);
- if (!enable) {
+ if (enable)
+ ret = transport_sas_phy_reset(phy, 0);
+ else {
sas_phy_disconnected(asd_phy);
sas_ha->notify_phy_event(asd_phy, PHYE_LOSS_OF_SIGNAL);
+ ret = i->dft->lldd_control_phy(asd_phy, cmd, NULL);
}
- ret = i->dft->lldd_control_phy(asd_phy, command, NULL);
} else {
struct sas_rphy *rphy = dev_to_rphy(phy->dev.parent);
struct domain_device *ddev = sas_find_dev_by_rphy(rphy);
- ret = sas_smp_phy_control(ddev, phy->number, command, NULL);
+
+ if (enable)
+ ret = transport_sas_phy_reset(phy, 0);
+ else
+ ret = sas_smp_phy_control(ddev, phy->number, cmd, NULL);
}
return ret;
}
diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c
index e51a909..2061e53 100644
--- a/drivers/scsi/libsas/sas_scsi_host.c
+++ b/drivers/scsi/libsas/sas_scsi_host.c
@@ -1083,7 +1083,6 @@ EXPORT_SYMBOL_GPL(sas_change_queue_type);
EXPORT_SYMBOL_GPL(sas_bios_param);
EXPORT_SYMBOL_GPL(sas_task_abort);
EXPORT_SYMBOL_GPL(sas_phy_reset);
-EXPORT_SYMBOL_GPL(sas_phy_enable);
EXPORT_SYMBOL_GPL(sas_eh_device_reset_handler);
EXPORT_SYMBOL_GPL(sas_eh_bus_reset_handler);
EXPORT_SYMBOL_GPL(sas_slave_alloc);
diff --git a/drivers/scsi/scsi_transport_sas.c b/drivers/scsi/scsi_transport_sas.c
index 760b80b..45ab52b 100644
--- a/drivers/scsi/scsi_transport_sas.c
+++ b/drivers/scsi/scsi_transport_sas.c
@@ -556,20 +556,20 @@ show_sas_device_type(struct device *dev,
}
static DEVICE_ATTR(device_type, S_IRUGO, show_sas_device_type, NULL);
-static ssize_t do_sas_phy_enable(struct device *dev,
- size_t count, int enable)
+static ssize_t do_sas_phy_enable(struct device *dev, size_t count, int enable)
{
struct sas_phy *phy = transport_class_to_phy(dev);
struct Scsi_Host *shost = dev_to_shost(phy->dev.parent);
- struct sas_internal *i = to_sas_internal(shost->transportt);
- int error;
- error = i->f->phy_enable(phy, enable);
- if (error)
- return error;
+ phy->enable = enable;
+ phy->enable_result = 0;
+ scsi_queue_work(shost, &phy->enable_work);
+ scsi_flush_work(shost);
+ if (phy->enable_result)
+ return phy->enable_result;
phy->enabled = enable;
return count;
-};
+}
static ssize_t
store_sas_phy_enable(struct device *dev, struct device_attribute *attr,
@@ -695,6 +695,15 @@ static void sas_phy_reset_work(struct work_struct *work)
phy->reset_result = i->f->phy_reset(phy, phy->hard_reset);
}
+static void sas_phy_enable_work(struct work_struct *work)
+{
+ struct sas_phy *phy = container_of(work, typeof(*phy), enable_work);
+ struct Scsi_Host *shost = dev_to_shost(phy->dev.parent);
+ struct sas_internal *i = to_sas_internal(shost->transportt);
+
+ phy->enable_result = i->f->phy_enable(phy, phy->enable);
+}
+
/**
* sas_phy_alloc - allocates and initialize a SAS PHY structure
* @parent: Parent device
@@ -724,6 +733,7 @@ struct sas_phy *sas_phy_alloc(struct device *parent, int number)
phy->dev.release = sas_phy_release;
INIT_LIST_HEAD(&phy->port_siblings);
INIT_WORK(&phy->reset_work, sas_phy_reset_work);
+ INIT_WORK(&phy->enable_work, sas_phy_enable_work);
if (scsi_is_sas_expander_device(parent)) {
struct sas_rphy *rphy = dev_to_rphy(parent);
dev_set_name(&phy->dev, "phy-%d:%d:%d", shost->host_no,
diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index 3ffc605..6e8f25d 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -624,7 +624,6 @@ extern int sas_unregister_ha(struct sas_ha_struct *);
int sas_set_phy_speed(struct sas_phy *phy,
struct sas_phy_linkrates *rates);
-int sas_phy_enable(struct sas_phy *phy, int enabled);
int sas_phy_reset(struct sas_phy *phy, int hard_reset);
int sas_queue_up(struct sas_task *task);
extern int sas_queuecommand(struct Scsi_Host * ,struct scsi_cmnd *);
diff --git a/include/scsi/scsi_transport_sas.h b/include/scsi/scsi_transport_sas.h
index c7eea0d..698e383 100644
--- a/include/scsi/scsi_transport_sas.h
+++ b/include/scsi/scsi_transport_sas.h
@@ -78,6 +78,9 @@ struct sas_phy {
int hard_reset;
int reset_result;
struct work_struct reset_work;
+ int enable;
+ int enable_result;
+ struct work_struct enable_work;
};
#define dev_to_phy(d) \
^ permalink raw reply related [flat|nested] 39+ messages in thread* [PATCH 21/24] libsas: Remove redundant phy state notification calls.
2011-12-17 2:33 [PATCH 00/24] libsas: eh reworks (ata-eh vs discovery, races, ...) Dan Williams
` (19 preceding siblings ...)
2011-12-17 2:34 ` [PATCH 20/24] libsas: sas_phy_enable via transport_sas_phy_reset Dan Williams
@ 2011-12-17 2:34 ` Dan Williams
2011-12-17 2:35 ` [PATCH 22/24] libsas: add mutex for SMP task execution Dan Williams
` (2 subsequent siblings)
23 siblings, 0 replies; 39+ messages in thread
From: Dan Williams @ 2011-12-17 2:34 UTC (permalink / raw)
To: linux-scsi
Cc: Xiangliang Yu, linux-ide, Jeff Skirvin, Luben Tuikov, Jack Wang
From: Jeff Skirvin <jeffrey.d.skirvin@intel.com>
In the case of an explicit sas_phy_enable call to disable a phy,
the LLDD provides the calls to sas_phy_disconnected and the
PHYE_LOSS_OF_SIGNAL event.
NOTE: This assumes that the lldd(s) generate the notification, which
appears to be the case, but only verfied on isci.
Cc: Jack Wang <jack_wang@usish.com>
Cc: Xiangliang Yu <yuxiangl@marvell.com>
Cc: Luben Tuikov <ltuikov@yahoo.com>
Signed-off-by: Jeff Skirvin <jeffrey.d.skirvin@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/scsi/libsas/sas_init.c | 5 +----
1 files changed, 1 insertions(+), 4 deletions(-)
diff --git a/drivers/scsi/libsas/sas_init.c b/drivers/scsi/libsas/sas_init.c
index e461b98..38fc521 100644
--- a/drivers/scsi/libsas/sas_init.c
+++ b/drivers/scsi/libsas/sas_init.c
@@ -264,11 +264,8 @@ static int sas_phy_enable(struct sas_phy *phy, int enable)
if (enable)
ret = transport_sas_phy_reset(phy, 0);
- else {
- sas_phy_disconnected(asd_phy);
- sas_ha->notify_phy_event(asd_phy, PHYE_LOSS_OF_SIGNAL);
+ else
ret = i->dft->lldd_control_phy(asd_phy, cmd, NULL);
- }
} else {
struct sas_rphy *rphy = dev_to_rphy(phy->dev.parent);
struct domain_device *ddev = sas_find_dev_by_rphy(rphy);
^ permalink raw reply related [flat|nested] 39+ messages in thread* [PATCH 22/24] libsas: add mutex for SMP task execution
2011-12-17 2:33 [PATCH 00/24] libsas: eh reworks (ata-eh vs discovery, races, ...) Dan Williams
` (20 preceding siblings ...)
2011-12-17 2:34 ` [PATCH 21/24] libsas: Remove redundant phy state notification calls Dan Williams
@ 2011-12-17 2:35 ` Dan Williams
2011-12-17 2:35 ` [PATCH 23/24] libsas: async ata-eh Dan Williams
2011-12-17 2:35 ` [PATCH 24/24] libsas: poll for ata device readiness after reset Dan Williams
23 siblings, 0 replies; 39+ messages in thread
From: Dan Williams @ 2011-12-17 2:35 UTC (permalink / raw)
To: linux-scsi; +Cc: linux-ide, Jeff Skirvin
From: Jeff Skirvin <jeffrey.d.skirvin@intel.com>
SAS does not tag SMP requests, and at least one lldd (isci) does not permit
more than one in-flight request at a time.
Signed-off-by: Jeff Skirvin <jeffrey.d.skirvin@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/scsi/libsas/sas_discover.c | 1 +
drivers/scsi/libsas/sas_expander.c | 29 ++++++++++++++++-------------
include/scsi/libsas.h | 2 ++
3 files changed, 19 insertions(+), 13 deletions(-)
diff --git a/drivers/scsi/libsas/sas_discover.c b/drivers/scsi/libsas/sas_discover.c
index 3f41ab7..e7837f9 100644
--- a/drivers/scsi/libsas/sas_discover.c
+++ b/drivers/scsi/libsas/sas_discover.c
@@ -43,6 +43,7 @@ void sas_init_dev(struct domain_device *dev)
case EDGE_DEV:
case FANOUT_DEV:
INIT_LIST_HEAD(&dev->ex_dev.children);
+ mutex_init(&dev->ex_dev.cmd_mutex);
break;
case SATA_DEV:
case SATA_PM:
diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index 9d2bb32..fd77ea3 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -72,11 +72,13 @@ static int smp_execute_task(struct domain_device *dev, void *req, int req_size,
struct sas_internal *i =
to_sas_internal(dev->port->ha->core.shost->transportt);
+ mutex_lock(&dev->ex_dev.cmd_mutex);
for (retry = 0; retry < 3; retry++) {
task = sas_alloc_task(GFP_KERNEL);
- if (!task)
- return -ENOMEM;
-
+ if (!task) {
+ res = -ENOMEM;
+ break;
+ }
task->dev = dev;
task->task_proto = dev->tproto;
sg_init_one(&task->smp_task.smp_req, req, req_size);
@@ -94,7 +96,7 @@ static int smp_execute_task(struct domain_device *dev, void *req, int req_size,
if (res) {
del_timer(&task->timer);
SAS_DPRINTK("executing SMP task failed:%d\n", res);
- goto ex_err;
+ break;
}
wait_for_completion(&task->completion);
@@ -104,21 +106,23 @@ static int smp_execute_task(struct domain_device *dev, void *req, int req_size,
i->dft->lldd_abort_task(task);
if (!(task->task_state_flags & SAS_TASK_STATE_DONE)) {
SAS_DPRINTK("SMP task aborted and not done\n");
- goto ex_err;
+ break;
}
}
if (task->task_status.resp == SAS_TASK_COMPLETE &&
task->task_status.stat == SAM_STAT_GOOD) {
res = 0;
break;
- } if (task->task_status.resp == SAS_TASK_COMPLETE &&
- task->task_status.stat == SAS_DATA_UNDERRUN) {
+ }
+ if (task->task_status.resp == SAS_TASK_COMPLETE &&
+ task->task_status.stat == SAS_DATA_UNDERRUN) {
/* no error, but return the number of bytes of
* underrun */
res = task->task_status.residual;
break;
- } if (task->task_status.resp == SAS_TASK_COMPLETE &&
- task->task_status.stat == SAS_DATA_OVERRUN) {
+ }
+ if (task->task_status.resp == SAS_TASK_COMPLETE &&
+ task->task_status.stat == SAS_DATA_OVERRUN) {
res = -EMSGSIZE;
break;
} else {
@@ -131,11 +135,10 @@ static int smp_execute_task(struct domain_device *dev, void *req, int req_size,
task = NULL;
}
}
-ex_err:
+ mutex_unlock(&dev->ex_dev.cmd_mutex);
+
BUG_ON(retry == 3 && task != NULL);
- if (task != NULL) {
- sas_free_task(task);
- }
+ sas_free_task(task);
return res;
}
diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index 6e8f25d..a8159f9 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -152,6 +152,8 @@ struct expander_device {
struct ex_phy *ex_phy;
struct sas_port *parent_port;
+
+ struct mutex cmd_mutex;
};
/* ---------- SATA device ---------- */
^ permalink raw reply related [flat|nested] 39+ messages in thread* [PATCH 23/24] libsas: async ata-eh
2011-12-17 2:33 [PATCH 00/24] libsas: eh reworks (ata-eh vs discovery, races, ...) Dan Williams
` (21 preceding siblings ...)
2011-12-17 2:35 ` [PATCH 22/24] libsas: add mutex for SMP task execution Dan Williams
@ 2011-12-17 2:35 ` Dan Williams
2011-12-17 2:35 ` [PATCH 24/24] libsas: poll for ata device readiness after reset Dan Williams
23 siblings, 0 replies; 39+ messages in thread
From: Dan Williams @ 2011-12-17 2:35 UTC (permalink / raw)
To: linux-scsi; +Cc: linux-ide
Once sas_ata_hard_reset() starts honoring the 'deadline' parameter a
pathological configuration could take 25 seconds per ata device
(serialized) to recover. Run per-port recoveries in parallel.
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/scsi/libsas/sas_ata.c | 17 ++++++++++++++---
1 files changed, 14 insertions(+), 3 deletions(-)
diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index 1df6ed2..773e010 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -23,6 +23,7 @@
#include <linux/scatterlist.h>
#include <linux/slab.h>
+#include <linux/async.h>
#include <scsi/sas_ata.h>
#include "sas_internal.h"
@@ -601,22 +602,32 @@ int sas_discover_sata(struct domain_device *dev)
return 0;
}
+static void async_sas_ata_eh(void *data, async_cookie_t cookie)
+{
+ struct domain_device *dev = data;
+ struct ata_port *ap = dev->sata_dev.ap;
+ struct sas_ha_struct *ha = dev->port->ha;
+
+ ata_port_printk(ap, KERN_DEBUG, "sas eh calling libata port error handler");
+ ata_scsi_port_error_handler(ha->core.shost, ap);
+}
+
void sas_ata_strategy_handler(struct Scsi_Host *shost)
{
struct scsi_device *sdev;
struct sas_ha_struct *sas_ha = SHOST_TO_SAS_HA(shost);
+ LIST_HEAD(async);
mutex_lock(&sas_ha->eh_mutex);
shost_for_each_device(sdev, shost) {
struct domain_device *ddev = sdev_to_domain_dev(sdev);
- struct ata_port *ap = ddev->sata_dev.ap;
if (!dev_is_sata(ddev))
continue;
- ata_port_printk(ap, KERN_DEBUG, "sas eh calling libata port error handler");
- ata_scsi_port_error_handler(shost, ap);
+ async_schedule_domain(async_sas_ata_eh, ddev, &async);
}
+ async_synchronize_full_domain(&async);
mutex_unlock(&sas_ha->eh_mutex);
}
^ permalink raw reply related [flat|nested] 39+ messages in thread* [PATCH 24/24] libsas: poll for ata device readiness after reset
2011-12-17 2:33 [PATCH 00/24] libsas: eh reworks (ata-eh vs discovery, races, ...) Dan Williams
` (22 preceding siblings ...)
2011-12-17 2:35 ` [PATCH 23/24] libsas: async ata-eh Dan Williams
@ 2011-12-17 2:35 ` Dan Williams
2011-12-20 6:18 ` Jack Wang
23 siblings, 1 reply; 39+ messages in thread
From: Dan Williams @ 2011-12-17 2:35 UTC (permalink / raw)
To: linux-scsi; +Cc: Tejun Heo, linux-ide
Use ata_wait_after_reset() to poll for link recovery after a reset.
This combined with sas_ha->eh_mutex prevents expander rediscovery from
probing phys in an intermediate state. Local discovery does not have a
mechanism to filter link status changes during this timeout, so it
remains the responsibility of lldds to prevent premature port teardown.
Although once all lldd's support ->lldd_ata_check_ready() that could be
used as a gate to local port teardown.
The signature fis is re-transmitted when the link comes back so we
should be revalidating the ata device class, but that is left to a future
patch.
Cc: Tejun Heo <tj@kernel.org>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/scsi/libsas/sas_ata.c | 104 +++++++++++++++++++++++++-----------
drivers/scsi/libsas/sas_expander.c | 10 ++-
drivers/scsi/libsas/sas_internal.h | 3 +
include/scsi/libsas.h | 1
4 files changed, 83 insertions(+), 35 deletions(-)
diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index 773e010..a5a7184 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -267,39 +267,84 @@ static bool sas_ata_qc_fill_rtf(struct ata_queued_cmd *qc)
return true;
}
-static int sas_ata_hard_reset(struct ata_link *link, unsigned int *class,
- unsigned long deadline)
+static struct sas_internal *dev_to_sas_internal(struct domain_device *dev)
+{
+ return to_sas_internal(dev->port->ha->core.shost->transportt);
+}
+
+static int smp_ata_check_ready(struct ata_link *link)
{
+ int res;
+ u8 addr[8];
struct ata_port *ap = link->ap;
struct domain_device *dev = ap->private_data;
- struct sas_internal *i =
- to_sas_internal(dev->port->ha->core.shost->transportt);
- int res = TMF_RESP_FUNC_FAILED;
- int ret = 0;
+ struct domain_device *ex_dev = dev->parent;
+ struct sas_phy *phy = sas_find_local_phy(dev);
- if (i->dft->lldd_I_T_nexus_reset)
- res = i->dft->lldd_I_T_nexus_reset(dev);
+ res = sas_get_phy_attached_sas_addr(ex_dev, phy->number, addr);
+ /* break the wait early if the expander is unreachable,
+ * otherwise keep polling
+ */
+ if (res == -ECOMM)
+ return res;
+ if (res != SMP_RESP_FUNC_ACC || SAS_ADDR(addr) == 0)
+ return 0;
+ else
+ return 1;
+}
- if (res != TMF_RESP_FUNC_COMPLETE) {
- SAS_DPRINTK("%s: Unable to reset I T nexus?\n", __func__);
- ret = -EAGAIN;
+static int local_ata_check_ready(struct ata_link *link)
+{
+ struct ata_port *ap = link->ap;
+ struct domain_device *dev = ap->private_data;
+ struct sas_internal *i = dev_to_sas_internal(dev);
+
+ if (i->dft->lldd_ata_check_ready)
+ return i->dft->lldd_ata_check_ready(dev);
+ else {
+ /* lldd's that don't implement 'ready' checking get the
+ * old default behavior of not coordinating reset
+ * recovery with libata
+ */
+ return 1;
}
+}
+static int sas_ata_hard_reset(struct ata_link *link, unsigned int *class,
+ unsigned long deadline)
+{
+ int ret = 0, res;
+ struct ata_port *ap = link->ap;
+ int (*check_ready)(struct ata_link *link);
+ struct domain_device *dev = ap->private_data;
+ struct sas_phy *phy = sas_find_local_phy(dev);
+ struct sas_internal *i = dev_to_sas_internal(dev);
+
+ res = i->dft->lldd_I_T_nexus_reset(dev);
+
+ if (res != TMF_RESP_FUNC_COMPLETE)
+ SAS_DPRINTK("%s: Unable to reset ata device?\n", __func__);
+
+ if (scsi_is_sas_phy_local(phy))
+ check_ready = local_ata_check_ready;
+ else
+ check_ready = smp_ata_check_ready;
+
+ ret = ata_wait_after_reset(link, deadline, check_ready);
+ if (ret && ret != -EAGAIN)
+ ata_link_err(link, "COMRESET failed (errno=%d)\n", ret);
+
+ /* XXX: if the class changes during the reset the upper layer
+ * should be informed, if the device has gone away we assume
+ * libsas will eventually delete it
+ */
switch (dev->sata_dev.command_set) {
- case ATA_COMMAND_SET:
- SAS_DPRINTK("%s: Found ATA device.\n", __func__);
- *class = ATA_DEV_ATA;
- break;
- case ATAPI_COMMAND_SET:
- SAS_DPRINTK("%s: Found ATAPI device.\n", __func__);
- *class = ATA_DEV_ATAPI;
- break;
- default:
- SAS_DPRINTK("%s: Unknown SATA command set: %d.\n",
- __func__,
- dev->sata_dev.command_set);
- *class = ATA_DEV_UNKNOWN;
- break;
+ case ATA_COMMAND_SET:
+ *class = ATA_DEV_ATA;
+ break;
+ case ATAPI_COMMAND_SET:
+ *class = ATA_DEV_ATAPI;
+ break;
}
ap->cbl = ATA_CBL_SATA;
@@ -311,8 +356,7 @@ static int sas_ata_soft_reset(struct ata_link *link, unsigned int *class,
{
struct ata_port *ap = link->ap;
struct domain_device *dev = ap->private_data;
- struct sas_internal *i =
- to_sas_internal(dev->port->ha->core.shost->transportt);
+ struct sas_internal *i = dev_to_sas_internal(dev);
int res = TMF_RESP_FUNC_FAILED;
int ret = 0;
@@ -350,8 +394,7 @@ static int sas_ata_soft_reset(struct ata_link *link, unsigned int *class,
*/
static void sas_ata_internal_abort(struct sas_task *task)
{
- struct sas_internal *si =
- to_sas_internal(task->dev->port->ha->core.shost->transportt);
+ struct sas_internal *si = dev_to_sas_internal(task->dev);
unsigned long flags;
int res;
@@ -421,8 +464,7 @@ static void sas_ata_post_internal(struct ata_queued_cmd *qc)
static void sas_ata_set_dmamode(struct ata_port *ap, struct ata_device *ata_dev)
{
struct domain_device *dev = ap->private_data;
- struct sas_internal *i =
- to_sas_internal(dev->port->ha->core.shost->transportt);
+ struct sas_internal *i = dev_to_sas_internal(dev);
if (i->dft->lldd_ata_set_dmamode)
i->dft->lldd_ata_set_dmamode(dev);
diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index fd77ea3..5e1eec9 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -125,7 +125,11 @@ static int smp_execute_task(struct domain_device *dev, void *req, int req_size,
task->task_status.stat == SAS_DATA_OVERRUN) {
res = -EMSGSIZE;
break;
- } else {
+ }
+ if (task->task_status.resp == SAS_TASK_UNDELIVERED &&
+ task->task_status.stat == SAS_DEVICE_UNKNOWN)
+ break;
+ else {
SAS_DPRINTK("%s: task to dev %016llx response: 0x%x "
"status 0x%x\n", __func__,
SAS_ADDR(dev->sas_addr),
@@ -1648,8 +1652,8 @@ static int sas_get_phy_change_count(struct domain_device *dev,
return res;
}
-static int sas_get_phy_attached_sas_addr(struct domain_device *dev,
- int phy_id, u8 *attached_sas_addr)
+int sas_get_phy_attached_sas_addr(struct domain_device *dev, int phy_id,
+ u8 *attached_sas_addr)
{
int res;
struct smp_resp *disc_resp;
diff --git a/drivers/scsi/libsas/sas_internal.h b/drivers/scsi/libsas/sas_internal.h
index 5879e72..dde4f29 100644
--- a/drivers/scsi/libsas/sas_internal.h
+++ b/drivers/scsi/libsas/sas_internal.h
@@ -74,7 +74,8 @@ int sas_smp_get_phy_events(struct sas_phy *phy);
struct domain_device *sas_find_dev_by_rphy(struct sas_rphy *rphy);
struct domain_device *sas_ex_to_ata(struct domain_device *ex_dev, int phy_id);
-
+int sas_get_phy_attached_sas_addr(struct domain_device *dev, int phy_id,
+ u8 *attached_sas_addr);
void sas_hae_reset(struct work_struct *work);
void sas_free_device(struct kref *kref);
diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index a8159f9..42110f9 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -605,6 +605,7 @@ struct sas_domain_function_template {
int (*lldd_clear_task_set)(struct domain_device *, u8 *lun);
int (*lldd_I_T_nexus_reset)(struct domain_device *);
int (*lldd_ata_soft_reset)(struct domain_device *);
+ int (*lldd_ata_check_ready)(struct domain_device *);
void (*lldd_ata_set_dmamode)(struct domain_device *);
int (*lldd_lu_reset)(struct domain_device *, u8 *lun);
int (*lldd_query_task)(struct sas_task *);
^ permalink raw reply related [flat|nested] 39+ messages in thread* RE: [PATCH 24/24] libsas: poll for ata device readiness after reset
2011-12-17 2:35 ` [PATCH 24/24] libsas: poll for ata device readiness after reset Dan Williams
@ 2011-12-20 6:18 ` Jack Wang
2011-12-20 7:08 ` Williams, Dan J
0 siblings, 1 reply; 39+ messages in thread
From: Jack Wang @ 2011-12-20 6:18 UTC (permalink / raw)
To: 'Dan Williams', linux-scsi; +Cc: 'Tejun Heo', linux-ide
I test the patchset ,it works ok.
You can add my tested-by if needed.
Jack
[PATCH 24/24] libsas: poll for ata device readiness after reset
>
> Use ata_wait_after_reset() to poll for link recovery after a reset.
> This combined with sas_ha->eh_mutex prevents expander rediscovery from
> probing phys in an intermediate state. Local discovery does not have a
> mechanism to filter link status changes during this timeout, so it
> remains the responsibility of lldds to prevent premature port teardown.
> Although once all lldd's support ->lldd_ata_check_ready() that could be
> used as a gate to local port teardown.
>
> The signature fis is re-transmitted when the link comes back so we
> should be revalidating the ata device class, but that is left to a future
> patch.
>
> Cc: Tejun Heo <tj@kernel.org>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
> drivers/scsi/libsas/sas_ata.c | 104
> +++++++++++++++++++++++++-----------
> drivers/scsi/libsas/sas_expander.c | 10 ++-
> drivers/scsi/libsas/sas_internal.h | 3 +
> include/scsi/libsas.h | 1
> 4 files changed, 83 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
> index 773e010..a5a7184 100644
> --- a/drivers/scsi/libsas/sas_ata.c
> +++ b/drivers/scsi/libsas/sas_ata.c
> @@ -267,39 +267,84 @@ static bool sas_ata_qc_fill_rtf(struct
ata_queued_cmd
> *qc)
> return true;
> }
>
> -static int sas_ata_hard_reset(struct ata_link *link, unsigned int *class,
> - unsigned long deadline)
> +static struct sas_internal *dev_to_sas_internal(struct domain_device
*dev)
> +{
> + return to_sas_internal(dev->port->ha->core.shost->transportt);
> +}
> +
> +static int smp_ata_check_ready(struct ata_link *link)
> {
> + int res;
> + u8 addr[8];
> struct ata_port *ap = link->ap;
> struct domain_device *dev = ap->private_data;
> - struct sas_internal *i =
> - to_sas_internal(dev->port->ha->core.shost->transportt);
> - int res = TMF_RESP_FUNC_FAILED;
> - int ret = 0;
> + struct domain_device *ex_dev = dev->parent;
> + struct sas_phy *phy = sas_find_local_phy(dev);
>
> - if (i->dft->lldd_I_T_nexus_reset)
> - res = i->dft->lldd_I_T_nexus_reset(dev);
> + res = sas_get_phy_attached_sas_addr(ex_dev, phy->number, addr);
> + /* break the wait early if the expander is unreachable,
> + * otherwise keep polling
> + */
> + if (res == -ECOMM)
> + return res;
> + if (res != SMP_RESP_FUNC_ACC || SAS_ADDR(addr) == 0)
> + return 0;
> + else
> + return 1;
> +}
>
> - if (res != TMF_RESP_FUNC_COMPLETE) {
> - SAS_DPRINTK("%s: Unable to reset I T nexus?\n", __func__);
> - ret = -EAGAIN;
> +static int local_ata_check_ready(struct ata_link *link)
> +{
> + struct ata_port *ap = link->ap;
> + struct domain_device *dev = ap->private_data;
> + struct sas_internal *i = dev_to_sas_internal(dev);
> +
> + if (i->dft->lldd_ata_check_ready)
> + return i->dft->lldd_ata_check_ready(dev);
> + else {
> + /* lldd's that don't implement 'ready' checking get the
> + * old default behavior of not coordinating reset
> + * recovery with libata
> + */
> + return 1;
> }
> +}
>
> +static int sas_ata_hard_reset(struct ata_link *link, unsigned int *class,
> + unsigned long deadline)
> +{
> + int ret = 0, res;
> + struct ata_port *ap = link->ap;
> + int (*check_ready)(struct ata_link *link);
> + struct domain_device *dev = ap->private_data;
> + struct sas_phy *phy = sas_find_local_phy(dev);
> + struct sas_internal *i = dev_to_sas_internal(dev);
> +
> + res = i->dft->lldd_I_T_nexus_reset(dev);
> +
> + if (res != TMF_RESP_FUNC_COMPLETE)
> + SAS_DPRINTK("%s: Unable to reset ata device?\n", __func__);
> +
> + if (scsi_is_sas_phy_local(phy))
> + check_ready = local_ata_check_ready;
> + else
> + check_ready = smp_ata_check_ready;
> +
> + ret = ata_wait_after_reset(link, deadline, check_ready);
> + if (ret && ret != -EAGAIN)
> + ata_link_err(link, "COMRESET failed (errno=%d)\n", ret);
> +
> + /* XXX: if the class changes during the reset the upper layer
> + * should be informed, if the device has gone away we assume
> + * libsas will eventually delete it
> + */
> switch (dev->sata_dev.command_set) {
> - case ATA_COMMAND_SET:
> - SAS_DPRINTK("%s: Found ATA device.\n", __func__);
> - *class = ATA_DEV_ATA;
> - break;
> - case ATAPI_COMMAND_SET:
> - SAS_DPRINTK("%s: Found ATAPI device.\n", __func__);
> - *class = ATA_DEV_ATAPI;
> - break;
> - default:
> - SAS_DPRINTK("%s: Unknown SATA command set: %d.\n",
> - __func__,
> - dev->sata_dev.command_set);
> - *class = ATA_DEV_UNKNOWN;
> - break;
> + case ATA_COMMAND_SET:
> + *class = ATA_DEV_ATA;
> + break;
> + case ATAPI_COMMAND_SET:
> + *class = ATA_DEV_ATAPI;
> + break;
> }
>
> ap->cbl = ATA_CBL_SATA;
> @@ -311,8 +356,7 @@ static int sas_ata_soft_reset(struct ata_link *link,
> unsigned int *class,
> {
> struct ata_port *ap = link->ap;
> struct domain_device *dev = ap->private_data;
> - struct sas_internal *i =
> - to_sas_internal(dev->port->ha->core.shost->transportt);
> + struct sas_internal *i = dev_to_sas_internal(dev);
> int res = TMF_RESP_FUNC_FAILED;
> int ret = 0;
>
> @@ -350,8 +394,7 @@ static int sas_ata_soft_reset(struct ata_link *link,
> unsigned int *class,
> */
> static void sas_ata_internal_abort(struct sas_task *task)
> {
> - struct sas_internal *si =
> -
to_sas_internal(task->dev->port->ha->core.shost->transportt);
> + struct sas_internal *si = dev_to_sas_internal(task->dev);
> unsigned long flags;
> int res;
>
> @@ -421,8 +464,7 @@ static void sas_ata_post_internal(struct
ata_queued_cmd
> *qc)
> static void sas_ata_set_dmamode(struct ata_port *ap, struct ata_device
> *ata_dev)
> {
> struct domain_device *dev = ap->private_data;
> - struct sas_internal *i =
> - to_sas_internal(dev->port->ha->core.shost->transportt);
> + struct sas_internal *i = dev_to_sas_internal(dev);
>
> if (i->dft->lldd_ata_set_dmamode)
> i->dft->lldd_ata_set_dmamode(dev);
> diff --git a/drivers/scsi/libsas/sas_expander.c
> b/drivers/scsi/libsas/sas_expander.c
> index fd77ea3..5e1eec9 100644
> --- a/drivers/scsi/libsas/sas_expander.c
> +++ b/drivers/scsi/libsas/sas_expander.c
> @@ -125,7 +125,11 @@ static int smp_execute_task(struct domain_device
*dev,
> void *req, int req_size,
> task->task_status.stat == SAS_DATA_OVERRUN) {
> res = -EMSGSIZE;
> break;
> - } else {
> + }
> + if (task->task_status.resp == SAS_TASK_UNDELIVERED &&
> + task->task_status.stat == SAS_DEVICE_UNKNOWN)
> + break;
> + else {
> SAS_DPRINTK("%s: task to dev %016llx response: 0x%x
"
> "status 0x%x\n", __func__,
> SAS_ADDR(dev->sas_addr),
> @@ -1648,8 +1652,8 @@ static int sas_get_phy_change_count(struct
> domain_device *dev,
> return res;
> }
>
> -static int sas_get_phy_attached_sas_addr(struct domain_device *dev,
> - int phy_id, u8 *attached_sas_addr)
> +int sas_get_phy_attached_sas_addr(struct domain_device *dev, int phy_id,
> + u8 *attached_sas_addr)
> {
> int res;
> struct smp_resp *disc_resp;
> diff --git a/drivers/scsi/libsas/sas_internal.h
> b/drivers/scsi/libsas/sas_internal.h
> index 5879e72..dde4f29 100644
> --- a/drivers/scsi/libsas/sas_internal.h
> +++ b/drivers/scsi/libsas/sas_internal.h
> @@ -74,7 +74,8 @@ int sas_smp_get_phy_events(struct sas_phy *phy);
>
> struct domain_device *sas_find_dev_by_rphy(struct sas_rphy *rphy);
> struct domain_device *sas_ex_to_ata(struct domain_device *ex_dev, int
> phy_id);
> -
> +int sas_get_phy_attached_sas_addr(struct domain_device *dev, int phy_id,
> + u8 *attached_sas_addr);
> void sas_hae_reset(struct work_struct *work);
>
> void sas_free_device(struct kref *kref);
> diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
> index a8159f9..42110f9 100644
> --- a/include/scsi/libsas.h
> +++ b/include/scsi/libsas.h
> @@ -605,6 +605,7 @@ struct sas_domain_function_template {
> int (*lldd_clear_task_set)(struct domain_device *, u8 *lun);
> int (*lldd_I_T_nexus_reset)(struct domain_device *);
> int (*lldd_ata_soft_reset)(struct domain_device *);
> + int (*lldd_ata_check_ready)(struct domain_device *);
> void (*lldd_ata_set_dmamode)(struct domain_device *);
> int (*lldd_lu_reset)(struct domain_device *, u8 *lun);
> int (*lldd_query_task)(struct sas_task *);
>
> --
> 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] 39+ messages in thread* Re: [PATCH 24/24] libsas: poll for ata device readiness after reset
2011-12-20 6:18 ` Jack Wang
@ 2011-12-20 7:08 ` Williams, Dan J
0 siblings, 0 replies; 39+ messages in thread
From: Williams, Dan J @ 2011-12-20 7:08 UTC (permalink / raw)
To: Jack Wang; +Cc: linux-scsi, Tejun Heo, linux-ide
On Mon, Dec 19, 2011 at 10:18 PM, Jack Wang <jack_wang@usish.com> wrote:
> I test the patchset ,it works ok.
> You can add my tested-by if needed.
>
> Jack
>
Thank you Jack.
Very much appreciated.
I will need to refresh the patches to address Tejun's comments about
drain_workqueue(), I'll get them re-posted shortly.
^ permalink raw reply [flat|nested] 39+ messages in thread