* [PATCH v3] libsas: export sas_alloc_task()
@ 2011-07-30 0:26 Dan Williams
2011-08-31 13:44 ` [PATCH] [SCSI]: Allow expander T-T attachments Mark Salyzyn
0 siblings, 1 reply; 13+ messages in thread
From: Dan Williams @ 2011-07-30 0:26 UTC (permalink / raw)
To: JBottomley; +Cc: Lindar Liu, Xiangliang Yu, Ankit Jain, linux-scsi, Jack Wang
Now that isci has added a 3rd open coded user of this functionality just
share the libsas version.
Acked-by: Jack Wang <jack_wang@usish.com>
Cc: Lindar Liu <lindar_liu@usish.com>
Cc: Xiangliang Yu <yuxiangl@marvell.com>
Cc: Ankit Jain <jankit@suse.de>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
Updated with Jack's ack, and now includes the removal of the open-coded
sas_alloc_task in isci.
drivers/scsi/isci/task.c | 29 +++--------------------------
drivers/scsi/libsas/sas_init.c | 30 +++++++++++++++++++++++++++---
drivers/scsi/mvsas/mv_sas.c | 29 +++--------------------------
drivers/scsi/pm8001/pm8001_sas.c | 38 ++++++--------------------------------
include/scsi/libsas.h | 26 ++------------------------
5 files changed, 41 insertions(+), 111 deletions(-)
diff --git a/drivers/scsi/isci/task.c b/drivers/scsi/isci/task.c
index d6bcdd0..e2d9418 100644
--- a/drivers/scsi/isci/task.c
+++ b/drivers/scsi/isci/task.c
@@ -1345,29 +1345,6 @@ static void isci_smp_task_done(struct sas_task *task)
complete(&task->completion);
}
-static struct sas_task *isci_alloc_task(void)
-{
- struct sas_task *task = kzalloc(sizeof(*task), GFP_KERNEL);
-
- if (task) {
- INIT_LIST_HEAD(&task->list);
- spin_lock_init(&task->task_state_lock);
- task->task_state_flags = SAS_TASK_STATE_PENDING;
- init_timer(&task->timer);
- init_completion(&task->completion);
- }
-
- return task;
-}
-
-static void isci_free_task(struct isci_host *ihost, struct sas_task *task)
-{
- if (task) {
- BUG_ON(!list_empty(&task->list));
- kfree(task);
- }
-}
-
static int isci_smp_execute_task(struct isci_host *ihost,
struct domain_device *dev, void *req,
int req_size, void *resp, int resp_size)
@@ -1376,7 +1353,7 @@ static int isci_smp_execute_task(struct isci_host *ihost,
struct sas_task *task = NULL;
for (retry = 0; retry < 3; retry++) {
- task = isci_alloc_task();
+ task = sas_alloc_task(GFP_KERNEL);
if (!task)
return -ENOMEM;
@@ -1439,13 +1416,13 @@ static int isci_smp_execute_task(struct isci_host *ihost,
SAS_ADDR(dev->sas_addr),
task->task_status.resp,
task->task_status.stat);
- isci_free_task(ihost, task);
+ sas_free_task(task);
task = NULL;
}
}
ex_err:
BUG_ON(retry == 3 && task != NULL);
- isci_free_task(ihost, task);
+ sas_free_task(task);
return res;
}
diff --git a/drivers/scsi/libsas/sas_init.c b/drivers/scsi/libsas/sas_init.c
index 2dc5534..dd56ea8 100644
--- a/drivers/scsi/libsas/sas_init.c
+++ b/drivers/scsi/libsas/sas_init.c
@@ -37,7 +37,32 @@
#include "../scsi_sas_internal.h"
-struct kmem_cache *sas_task_cache;
+static struct kmem_cache *sas_task_cache;
+
+struct sas_task *sas_alloc_task(gfp_t flags)
+{
+ struct sas_task *task = kmem_cache_zalloc(sas_task_cache, flags);
+
+ if (task) {
+ INIT_LIST_HEAD(&task->list);
+ spin_lock_init(&task->task_state_lock);
+ task->task_state_flags = SAS_TASK_STATE_PENDING;
+ init_timer(&task->timer);
+ init_completion(&task->completion);
+ }
+
+ return task;
+}
+EXPORT_SYMBOL_GPL(sas_alloc_task);
+
+void sas_free_task(struct sas_task *task)
+{
+ if (task) {
+ BUG_ON(!list_empty(&task->list));
+ kmem_cache_free(sas_task_cache, task);
+ }
+}
+EXPORT_SYMBOL_GPL(sas_free_task);
/*------------ SAS addr hash -----------*/
void sas_hash_addr(u8 *hashed, const u8 *sas_addr)
@@ -293,8 +318,7 @@ EXPORT_SYMBOL_GPL(sas_domain_release_transport);
static int __init sas_class_init(void)
{
- sas_task_cache = kmem_cache_create("sas_task", sizeof(struct sas_task),
- 0, SLAB_HWCACHE_ALIGN, NULL);
+ sas_task_cache = KMEM_CACHE(sas_task, SLAB_HWCACHE_ALIGN);
if (!sas_task_cache)
return -ENOMEM;
diff --git a/drivers/scsi/mvsas/mv_sas.c b/drivers/scsi/mvsas/mv_sas.c
index 0ef2742..7921b61 100644
--- a/drivers/scsi/mvsas/mv_sas.c
+++ b/drivers/scsi/mvsas/mv_sas.c
@@ -1518,28 +1518,6 @@ void mvs_dev_gone(struct domain_device *dev)
mvs_dev_gone_notify(dev);
}
-static struct sas_task *mvs_alloc_task(void)
-{
- struct sas_task *task = kzalloc(sizeof(struct sas_task), GFP_KERNEL);
-
- if (task) {
- INIT_LIST_HEAD(&task->list);
- spin_lock_init(&task->task_state_lock);
- task->task_state_flags = SAS_TASK_STATE_PENDING;
- init_timer(&task->timer);
- init_completion(&task->completion);
- }
- return task;
-}
-
-static void mvs_free_task(struct sas_task *task)
-{
- if (task) {
- BUG_ON(!list_empty(&task->list));
- kfree(task);
- }
-}
-
static void mvs_task_done(struct sas_task *task)
{
if (!del_timer(&task->timer))
@@ -1564,7 +1542,7 @@ static int mvs_exec_internal_tmf_task(struct domain_device *dev,
struct sas_task *task = NULL;
for (retry = 0; retry < 3; retry++) {
- task = mvs_alloc_task();
+ task = sas_alloc_task(GFP_KERNEL);
if (!task)
return -ENOMEM;
@@ -1622,15 +1600,14 @@ static int mvs_exec_internal_tmf_task(struct domain_device *dev,
SAS_ADDR(dev->sas_addr),
task->task_status.resp,
task->task_status.stat);
- mvs_free_task(task);
+ sas_free_task(task);
task = NULL;
}
}
ex_err:
BUG_ON(retry == 3 && task != NULL);
- if (task != NULL)
- mvs_free_task(task);
+ sas_free_task(task);
return res;
}
diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c
index 6ae059e..7dbbf8b 100644
--- a/drivers/scsi/pm8001/pm8001_sas.c
+++ b/drivers/scsi/pm8001/pm8001_sas.c
@@ -669,30 +669,6 @@ int pm8001_dev_found(struct domain_device *dev)
return pm8001_dev_found_notify(dev);
}
-/**
- * pm8001_alloc_task - allocate a task structure for TMF
- */
-static struct sas_task *pm8001_alloc_task(void)
-{
- struct sas_task *task = kzalloc(sizeof(*task), GFP_KERNEL);
- if (task) {
- INIT_LIST_HEAD(&task->list);
- spin_lock_init(&task->task_state_lock);
- task->task_state_flags = SAS_TASK_STATE_PENDING;
- init_timer(&task->timer);
- init_completion(&task->completion);
- }
- return task;
-}
-
-static void pm8001_free_task(struct sas_task *task)
-{
- if (task) {
- BUG_ON(!list_empty(&task->list));
- kfree(task);
- }
-}
-
static void pm8001_task_done(struct sas_task *task)
{
if (!del_timer(&task->timer))
@@ -728,7 +704,7 @@ static int pm8001_exec_internal_tmf_task(struct domain_device *dev,
struct pm8001_hba_info *pm8001_ha = pm8001_find_ha_by_dev(dev);
for (retry = 0; retry < 3; retry++) {
- task = pm8001_alloc_task();
+ task = sas_alloc_task(GFP_KERNEL);
if (!task)
return -ENOMEM;
@@ -789,14 +765,13 @@ static int pm8001_exec_internal_tmf_task(struct domain_device *dev,
SAS_ADDR(dev->sas_addr),
task->task_status.resp,
task->task_status.stat));
- pm8001_free_task(task);
+ sas_free_task(task);
task = NULL;
}
}
ex_err:
BUG_ON(retry == 3 && task != NULL);
- if (task != NULL)
- pm8001_free_task(task);
+ sas_free_task(task);
return res;
}
@@ -811,7 +786,7 @@ pm8001_exec_internal_task_abort(struct pm8001_hba_info *pm8001_ha,
struct sas_task *task = NULL;
for (retry = 0; retry < 3; retry++) {
- task = pm8001_alloc_task();
+ task = sas_alloc_task(GFP_KERNEL);
if (!task)
return -ENOMEM;
@@ -864,14 +839,13 @@ pm8001_exec_internal_task_abort(struct pm8001_hba_info *pm8001_ha,
SAS_ADDR(dev->sas_addr),
task->task_status.resp,
task->task_status.stat));
- pm8001_free_task(task);
+ sas_free_task(task);
task = NULL;
}
}
ex_err:
BUG_ON(retry == 3 && task != NULL);
- if (task != NULL)
- pm8001_free_task(task);
+ sas_free_task(task);
return res;
}
diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index ee86606..2517254 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -555,36 +555,14 @@ struct sas_task {
struct work_struct abort_work;
};
-extern struct kmem_cache *sas_task_cache;
-
#define SAS_TASK_STATE_PENDING 1
#define SAS_TASK_STATE_DONE 2
#define SAS_TASK_STATE_ABORTED 4
#define SAS_TASK_NEED_DEV_RESET 8
#define SAS_TASK_AT_INITIATOR 16
-static inline struct sas_task *sas_alloc_task(gfp_t flags)
-{
- struct sas_task *task = kmem_cache_zalloc(sas_task_cache, flags);
-
- if (task) {
- INIT_LIST_HEAD(&task->list);
- spin_lock_init(&task->task_state_lock);
- task->task_state_flags = SAS_TASK_STATE_PENDING;
- init_timer(&task->timer);
- init_completion(&task->completion);
- }
-
- return task;
-}
-
-static inline void sas_free_task(struct sas_task *task)
-{
- if (task) {
- BUG_ON(!list_empty(&task->list));
- kmem_cache_free(sas_task_cache, task);
- }
-}
+extern struct sas_task *sas_alloc_task(gfp_t flags);
+extern void sas_free_task(struct sas_task *task);
struct sas_domain_function_template {
/* The class calls these to notify the LLDD of an event. */
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH] [SCSI]: Allow expander T-T attachments
2011-07-30 0:26 [PATCH v3] libsas: export sas_alloc_task() Dan Williams
@ 2011-08-31 13:44 ` Mark Salyzyn
2011-08-31 17:43 ` Luben Tuikov
2011-09-01 13:11 ` [PATCH] [SCSI]: libsas failure to revalidate domain for anything but the first expander child Mark Salyzyn
0 siblings, 2 replies; 13+ messages in thread
From: Mark Salyzyn @ 2011-08-31 13:44 UTC (permalink / raw)
To: linux-scsi; +Cc: Luben Tuikov, Darrick J Wong, James Bottomley
[-- Attachment #1: Type: text/plain, Size: 5157 bytes --]
Since Luben's patch on July 28th 2011 went nowhere, as there was an
unanswered question regarding the future-proofing that Luben added in. I
decided to submit a simplified focussed patch we have been using for the
past year testing with the Xyratex enclosures (5U84, 2U24, chained with
*many* peered expanders with Table to Table routing) that should
hopefully expedite things and will be on hot standby to refactor as
requested (including tossing this baby and refactoring Luben's patch
should he be too busy, no hair off my back)
Please note, as I am *stuck* on Outlook as per company policy, the
following inline content will likely not patch clean even emailed as
'Plain Text', the enclosed attached file should do the job. I have Cc'd
all the folks that originated the files in libsas, as there was no
listed MAINTAINERs.
NB: this patch (and Luben's independent patch) results in an ABI break
as the structures change 'shape' and thus result in a different set of
libsas export signatures. I have an internal patch I use that preserves
the structure shapes and thus the ABI; but would be considered
inappropriate for the pristine trees. Said alternate patch would work
fine for a Distribution tree where ABI concerns are an issue.
Checkpatch.pl reports clean.
Sincerely -- Mark Salyzyn
Cc: Luben Tuikov <tuikov@yahoo.com>
Cc: Darrick J Wong <djwong@us.ibm.com>
Cc: James Bottomley <jbottomley@parallels.com>
Signed-off-by: Mark Salyzyn <msalyzyn@us.xyratex.com>
drivers/scsi/libsas/sas_expander.c | 6 +++++-
include/scsi/libsas.h | 1 +
include/scsi/sas.h | 6 ++++--
3 files changed, 10 insertions(+), 3 deletions(-)
diff -ru scsi-misc-2.6/drivers/scsi/libsas/sas_expander.c
scsi-misc-2.6.new/drivers/scsi/libsas/sas_expander.c
--- scsi-misc-2.6/drivers/scsi/libsas/sas_expander.c 2011-08-31
08:32:21.000000000 -0400
+++ scsi-misc-2.6.new/drivers/scsi/libsas/sas_expander.c
2011-08-31 09:07:31.000000000 -0400
@@ -331,6 +331,7 @@
dev->ex_dev.num_phys = min(rg->num_phys, (u8)MAX_EXPANDER_PHYS);
dev->ex_dev.conf_route_table = rg->conf_route_table;
dev->ex_dev.configuring = rg->configuring;
+ dev->ex_dev.table_to_table = rg->table_to_table;
memcpy(dev->ex_dev.enclosure_logical_id,
rg->enclosure_logical_id, 8);
}
@@ -1239,7 +1240,10 @@
res = -ENODEV;
}
} else if (parent_phy->routing_attr ==
TABLE_ROUTING &&
- child_phy->routing_attr !=
SUBTRACTIVE_ROUTING) {
+ child_phy->routing_attr !=
+ SUBTRACTIVE_ROUTING &&
+ (child_ex->table_to_table == 0 ||
+ child_phy->routing_attr !=
TABLE_ROUTING)) {
sas_print_parent_topology_bug(child,
parent_phy, child_phy);
res = -ENODEV;
}
diff -ru scsi-misc-2.6/include/scsi/libsas.h
scsi-misc-2.6.new/include/scsi/libsas.h
--- scsi-misc-2.6/include/scsi/libsas.h 2011-08-31 08:32:22.000000000
-0400
+++ scsi-misc-2.6.new/include/scsi/libsas.h 2011-08-31
09:07:31.000000000 -0400
@@ -144,6 +144,7 @@
u8 num_phys;
u8 configuring:1;
u8 conf_route_table:1;
+ u8 table_to_table:1;
u8 enclosure_logical_id[8];
struct ex_phy *ex_phy;
diff -ru scsi-misc-2.6/include/scsi/sas.h
scsi-misc-2.6.new/include/scsi/sas.h
--- scsi-misc-2.6/include/scsi/sas.h 2011-08-31 08:32:22.000000000
-0400
+++ scsi-misc-2.6.new/include/scsi/sas.h 2011-08-31
09:07:31.000000000 -0400
@@ -341,7 +341,8 @@
u8 conf_route_table:1;
u8 configuring:1;
- u8 _r_b:6;
+ u8 _r_b:5;
+ u8 table_to_table:1;
u8 _r_c;
@@ -528,7 +529,8 @@
u8 _r_a;
u8 num_phys;
- u8 _r_b:6;
+ u8 table_to_table:1;
+ u8 _r_b:5;
u8 configuring:1;
u8 conf_route_table:1;
______________________________________________________________________
This email may contain privileged or confidential information, which should only be used for the purpose for which it was sent by Xyratex. No further rights or licenses are granted to use such information. If you are not the intended recipient of this message, please notify the sender by return and delete it. You may not use, copy, disclose or rely on the information contained in it.
Internet email is susceptible to data corruption, interception and unauthorised amendment for which Xyratex does not accept liability. While we have taken reasonable precautions to ensure that this email is free of viruses, Xyratex does not accept liability for the presence of any computer viruses in this email, nor for any losses caused as a result of viruses.
Xyratex Technology Limited (03134912), Registered in England & Wales, Registered Office, Langstone Road, Havant, Hampshire, PO9 1SA.
The Xyratex group of companies also includes, Xyratex Ltd, registered in Bermuda, Xyratex International Inc, registered in California, Xyratex (Malaysia) Sdn Bhd registered in Malaysia, Xyratex Technology (Wuxi) Co Ltd registered in The People's Republic of China and Xyratex Japan Limited registered in Japan.
______________________________________________________________________
\r
[-- Attachment #2: table_to_table.patch --]
[-- Type: application/octet-stream, Size: 2272 bytes --]
Signed-off-by: Mark Salyzyn <msalyzyn@us.xyratex.com>
drivers/scsi/libsas/sas_expander.c | 6 +++++-
include/scsi/libsas.h | 1 +
include/scsi/sas.h | 6 ++++--
3 files changed, 10 insertions(+), 3 deletions(-)
diff -ru scsi-misc-2.6/drivers/scsi/libsas/sas_expander.c scsi-misc-2.6.new/drivers/scsi/libsas/sas_expander.c
--- scsi-misc-2.6/drivers/scsi/libsas/sas_expander.c 2011-08-31 08:32:21.000000000 -0400
+++ scsi-misc-2.6.new/drivers/scsi/libsas/sas_expander.c 2011-08-31 09:07:31.000000000 -0400
@@ -331,6 +331,7 @@
dev->ex_dev.num_phys = min(rg->num_phys, (u8)MAX_EXPANDER_PHYS);
dev->ex_dev.conf_route_table = rg->conf_route_table;
dev->ex_dev.configuring = rg->configuring;
+ dev->ex_dev.table_to_table = rg->table_to_table;
memcpy(dev->ex_dev.enclosure_logical_id, rg->enclosure_logical_id, 8);
}
@@ -1239,7 +1240,10 @@
res = -ENODEV;
}
} else if (parent_phy->routing_attr == TABLE_ROUTING &&
- child_phy->routing_attr != SUBTRACTIVE_ROUTING) {
+ child_phy->routing_attr !=
+ SUBTRACTIVE_ROUTING &&
+ (child_ex->table_to_table == 0 ||
+ child_phy->routing_attr != TABLE_ROUTING)) {
sas_print_parent_topology_bug(child, parent_phy, child_phy);
res = -ENODEV;
}
diff -ru scsi-misc-2.6/include/scsi/libsas.h scsi-misc-2.6.new/include/scsi/libsas.h
--- scsi-misc-2.6/include/scsi/libsas.h 2011-08-31 08:32:22.000000000 -0400
+++ scsi-misc-2.6.new/include/scsi/libsas.h 2011-08-31 09:07:31.000000000 -0400
@@ -144,6 +144,7 @@
u8 num_phys;
u8 configuring:1;
u8 conf_route_table:1;
+ u8 table_to_table:1;
u8 enclosure_logical_id[8];
struct ex_phy *ex_phy;
diff -ru scsi-misc-2.6/include/scsi/sas.h scsi-misc-2.6.new/include/scsi/sas.h
--- scsi-misc-2.6/include/scsi/sas.h 2011-08-31 08:32:22.000000000 -0400
+++ scsi-misc-2.6.new/include/scsi/sas.h 2011-08-31 09:07:31.000000000 -0400
@@ -341,7 +341,8 @@
u8 conf_route_table:1;
u8 configuring:1;
- u8 _r_b:6;
+ u8 _r_b:5;
+ u8 table_to_table:1;
u8 _r_c;
@@ -528,7 +529,8 @@
u8 _r_a;
u8 num_phys;
- u8 _r_b:6;
+ u8 table_to_table:1;
+ u8 _r_b:5;
u8 configuring:1;
u8 conf_route_table:1;
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] [SCSI]: Allow expander T-T attachments
2011-08-31 13:44 ` [PATCH] [SCSI]: Allow expander T-T attachments Mark Salyzyn
@ 2011-08-31 17:43 ` Luben Tuikov
2011-08-31 17:51 ` Mark Salyzyn
2011-09-01 13:11 ` [PATCH] [SCSI]: libsas failure to revalidate domain for anything but the first expander child Mark Salyzyn
1 sibling, 1 reply; 13+ messages in thread
From: Luben Tuikov @ 2011-08-31 17:43 UTC (permalink / raw)
To: Mark Salyzyn, linux-scsi; +Cc: Darrick J Wong, James Bottomley
----- Original Message -----
> From: Mark Salyzyn <mark_salyzyn@us.xyratex.com>
> To: linux-scsi@vger.kernel.org
> Cc: Luben Tuikov <ltuikov@yahoo.com>; Darrick J Wong <djwong@us.ibm.com>; James Bottomley <jbottomley@parallels.com>
> Sent: Wednesday, August 31, 2011 6:44 AM
> Subject: Re: [PATCH] [SCSI]: Allow expander T-T attachments
>
> Since Luben's patch on July 28th 2011 went nowhere, as there was an
> unanswered question regarding the future-proofing that Luben added in.
Are you saying that the patch I posted,
http://marc.info/?t=131182720200001&r=1&w=2, didn't go in
because I didn't entertain Bottomley's unrelated question therein?
Or are you saying that it did go in based on TECHNICAL CONTENT? I.e. it
was buggy, or wrong, etc?
> I decided to submit a simplified focussed patch we have been using for the
By "simplified, focused" you mean one which doesn't define 2 out of 3 flags of
REPORT GENERAL SMP response?
> past year testing with the Xyratex enclosures (5U84, 2U24, chained with
> *many* peered expanders with Table to Table routing) that should
> hopefully expedite things and will be on hot standby to refactor as
> requested (including tossing this baby and refactoring Luben's patch
> should he be too busy, no hair off my back)
Now onto technical matter:
1) I'd call the flag "t2t_supp" to a) fit in with the rest of the spirit of the
code as other fields are also called "_supp", and to b) actually convey
the meaning of "table-to-table supported. Your naming of this flag
"table_to_table" implicitly means "table to table supported".
2) Please also add the debug output as my patch did. People will
be wondering what went wrong if their domain didn't work, and the
debug print in my original patch would tell them that.
3) You don't check that the parent also supports T2T. That's a bug.
4) Your patch logically does this: P=T && C!=S && (C is T2T || C!=T),
which after expansion is equivalent to:
(P==T) && ((C!=S && C!=T2T) || C==D) ==> fail
where the outmost parens are an if (). Albeit from not being entirely correct,
it also obfuscates what is being sought after. I chose to use the negation for
successful check to make it more clear:
(P==T) && (C==S || (C==T && P==T2T && C==T2T)) ==> success,
Translates to
"If parent phy is T, and If child phy is S OR (child is T and child and parent
both support table to table) then we're okay, else report an error."
Which is: T<-S is okay, and T<-T is okay if both parent and child support it.
Alternatively, Bottomley could just add my patch upstream.
> Please note, as I am *stuck* on Outlook as per company policy, the
> following inline content will likely not patch clean even emailed as
> 'Plain Text', the enclosed attached file should do the job. I have
> Cc'd
> all the folks that originated the files in libsas, as there was no
> listed MAINTAINERs.
>
> NB: this patch (and Luben's independent patch) results in an ABI break
> as the structures change 'shape' and thus result in a different set of
> libsas export signatures. I have an internal patch I use that preserves
> the structure shapes and thus the ABI; but would be considered
> inappropriate for the pristine trees. Said alternate patch would work
> fine for a Distribution tree where ABI concerns are an issue.
>
> Checkpatch.pl reports clean.
>
> Sincerely -- Mark Salyzyn
>
> Cc: Luben Tuikov <tuikov@yahoo.com>
> Cc: Darrick J Wong <djwong@us.ibm.com>
> Cc: James Bottomley <jbottomley@parallels.com>
>
> Signed-off-by: Mark Salyzyn <msalyzyn@us.xyratex.com>
>
> drivers/scsi/libsas/sas_expander.c | 6 +++++-
> include/scsi/libsas.h | 1 +
> include/scsi/sas.h | 6 ++++--
> 3 files changed, 10 insertions(+), 3 deletions(-)
>
> diff -ru scsi-misc-2.6/drivers/scsi/libsas/sas_expander.c
> scsi-misc-2.6.new/drivers/scsi/libsas/sas_expander.c
> --- scsi-misc-2.6/drivers/scsi/libsas/sas_expander.c 2011-08-31
> 08:32:21.000000000 -0400
> +++ scsi-misc-2.6.new/drivers/scsi/libsas/sas_expander.c
> 2011-08-31 09:07:31.000000000 -0400
> @@ -331,6 +331,7 @@
> dev->ex_dev.num_phys = min(rg->num_phys, (u8)MAX_EXPANDER_PHYS);
> dev->ex_dev.conf_route_table = rg->conf_route_table;
> dev->ex_dev.configuring = rg->configuring;
> + dev->ex_dev.table_to_table = rg->table_to_table;
> memcpy(dev->ex_dev.enclosure_logical_id,
> rg->enclosure_logical_id, 8);
> }
>
> @@ -1239,7 +1240,10 @@
> res = -ENODEV;
> }
> } else if (parent_phy->routing_attr ==
> TABLE_ROUTING &&
> - child_phy->routing_attr !=
> SUBTRACTIVE_ROUTING) {
> + child_phy->routing_attr !=
> + SUBTRACTIVE_ROUTING &&
> + (child_ex->table_to_table == 0 ||
> + child_phy->routing_attr !=
> TABLE_ROUTING)) {
> sas_print_parent_topology_bug(child,
> parent_phy, child_phy);
> res = -ENODEV;
> }
> diff -ru scsi-misc-2.6/include/scsi/libsas.h
> scsi-misc-2.6.new/include/scsi/libsas.h
> --- scsi-misc-2.6/include/scsi/libsas.h 2011-08-31 08:32:22.000000000
> -0400
> +++ scsi-misc-2.6.new/include/scsi/libsas.h 2011-08-31
> 09:07:31.000000000 -0400
> @@ -144,6 +144,7 @@
> u8 num_phys;
> u8 configuring:1;
> u8 conf_route_table:1;
> + u8 table_to_table:1;
> u8 enclosure_logical_id[8];
>
> struct ex_phy *ex_phy;
> diff -ru scsi-misc-2.6/include/scsi/sas.h
> scsi-misc-2.6.new/include/scsi/sas.h
> --- scsi-misc-2.6/include/scsi/sas.h 2011-08-31 08:32:22.000000000
> -0400
> +++ scsi-misc-2.6.new/include/scsi/sas.h 2011-08-31
> 09:07:31.000000000 -0400
> @@ -341,7 +341,8 @@
>
> u8 conf_route_table:1;
> u8 configuring:1;
> - u8 _r_b:6;
> + u8 _r_b:5;
> + u8 table_to_table:1;
>
> u8 _r_c;
>
> @@ -528,7 +529,8 @@
> u8 _r_a;
> u8 num_phys;
>
> - u8 _r_b:6;
> + u8 table_to_table:1;
> + u8 _r_b:5;
> u8 configuring:1;
> u8 conf_route_table:1;
>
--
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] 13+ messages in thread* RE: [PATCH] [SCSI]: Allow expander T-T attachments
2011-08-31 17:43 ` Luben Tuikov
@ 2011-08-31 17:51 ` Mark Salyzyn
2011-09-01 15:34 ` James Bottomley
2011-09-01 17:25 ` Luben Tuikov
0 siblings, 2 replies; 13+ messages in thread
From: Mark Salyzyn @ 2011-08-31 17:51 UTC (permalink / raw)
To: Luben Tuikov, linux-scsi; +Cc: Darrick J Wong, James Bottomley
We will need James to chime in why your patch did not go in. My assumption is the lack of response to his question left it in limbo.
The likely modification to your patch would be to remove all the unused added flags rather than staking out the territory.
What kind of testing was involved in your patch? I have not merged yours in here to try it out on our enclosures. Either way, I want to see some movement; James any directions?
Sincerely -- Mark Salyzyn
-----Original Message-----
From: Luben Tuikov [mailto:ltuikov@yahoo.com]
Sent: Wednesday, August 31, 2011 1:43 PM
To: Mark Salyzyn; linux-scsi@vger.kernel.org
Cc: Darrick J Wong; James Bottomley
Subject: Re: [PATCH] [SCSI]: Allow expander T-T attachments
----- Original Message -----
> From: Mark Salyzyn <mark_salyzyn@us.xyratex.com>
> To: linux-scsi@vger.kernel.org
> Cc: Luben Tuikov <ltuikov@yahoo.com>; Darrick J Wong <djwong@us.ibm.com>; James Bottomley <jbottomley@parallels.com>
> Sent: Wednesday, August 31, 2011 6:44 AM
> Subject: Re: [PATCH] [SCSI]: Allow expander T-T attachments
>
> Since Luben's patch on July 28th 2011 went nowhere, as there was an
> unanswered question regarding the future-proofing that Luben added in.
Are you saying that the patch I posted,
http://marc.info/?t=131182720200001&r=1&w=2, didn't go in
because I didn't entertain Bottomley's unrelated question therein?
Or are you saying that it did go in based on TECHNICAL CONTENT? I.e. it
was buggy, or wrong, etc?
> I decided to submit a simplified focussed patch we have been using for the
By "simplified, focused" you mean one which doesn't define 2 out of 3 flags of
REPORT GENERAL SMP response?
> past year testing with the Xyratex enclosures (5U84, 2U24, chained with
> *many* peered expanders with Table to Table routing) that should
> hopefully expedite things and will be on hot standby to refactor as
> requested (including tossing this baby and refactoring Luben's patch
> should he be too busy, no hair off my back)
Now onto technical matter:
1) I'd call the flag "t2t_supp" to a) fit in with the rest of the spirit of the
code as other fields are also called "_supp", and to b) actually convey
the meaning of "table-to-table supported. Your naming of this flag
"table_to_table" implicitly means "table to table supported".
2) Please also add the debug output as my patch did. People will
be wondering what went wrong if their domain didn't work, and the
debug print in my original patch would tell them that.
3) You don't check that the parent also supports T2T. That's a bug.
4) Your patch logically does this: P=T && C!=S && (C is T2T || C!=T),
which after expansion is equivalent to:
(P==T) && ((C!=S && C!=T2T) || C==D) ==> fail
where the outmost parens are an if (). Albeit from not being entirely correct,
it also obfuscates what is being sought after. I chose to use the negation for
successful check to make it more clear:
(P==T) && (C==S || (C==T && P==T2T && C==T2T)) ==> success,
Translates to
"If parent phy is T, and If child phy is S OR (child is T and child and parent
both support table to table) then we're okay, else report an error."
Which is: T<-S is okay, and T<-T is okay if both parent and child support it.
Alternatively, Bottomley could just add my patch upstream.
> Please note, as I am *stuck* on Outlook as per company policy, the
> following inline content will likely not patch clean even emailed as
> 'Plain Text', the enclosed attached file should do the job. I have
> Cc'd
> all the folks that originated the files in libsas, as there was no
> listed MAINTAINERs.
>
> NB: this patch (and Luben's independent patch) results in an ABI break
> as the structures change 'shape' and thus result in a different set of
> libsas export signatures. I have an internal patch I use that preserves
> the structure shapes and thus the ABI; but would be considered
> inappropriate for the pristine trees. Said alternate patch would work
> fine for a Distribution tree where ABI concerns are an issue.
>
> Checkpatch.pl reports clean.
>
> Sincerely -- Mark Salyzyn
>
> Cc: Luben Tuikov <tuikov@yahoo.com>
> Cc: Darrick J Wong <djwong@us.ibm.com>
> Cc: James Bottomley <jbottomley@parallels.com>
>
> Signed-off-by: Mark Salyzyn <msalyzyn@us.xyratex.com>
>
> drivers/scsi/libsas/sas_expander.c | 6 +++++-
> include/scsi/libsas.h | 1 +
> include/scsi/sas.h | 6 ++++--
> 3 files changed, 10 insertions(+), 3 deletions(-)
>
> diff -ru scsi-misc-2.6/drivers/scsi/libsas/sas_expander.c
> scsi-misc-2.6.new/drivers/scsi/libsas/sas_expander.c
> --- scsi-misc-2.6/drivers/scsi/libsas/sas_expander.c 2011-08-31
> 08:32:21.000000000 -0400
> +++ scsi-misc-2.6.new/drivers/scsi/libsas/sas_expander.c
> 2011-08-31 09:07:31.000000000 -0400
> @@ -331,6 +331,7 @@
> dev->ex_dev.num_phys = min(rg->num_phys, (u8)MAX_EXPANDER_PHYS);
> dev->ex_dev.conf_route_table = rg->conf_route_table;
> dev->ex_dev.configuring = rg->configuring;
> + dev->ex_dev.table_to_table = rg->table_to_table;
> memcpy(dev->ex_dev.enclosure_logical_id,
> rg->enclosure_logical_id, 8);
> }
>
> @@ -1239,7 +1240,10 @@
> res = -ENODEV;
> }
> } else if (parent_phy->routing_attr ==
> TABLE_ROUTING &&
> - child_phy->routing_attr !=
> SUBTRACTIVE_ROUTING) {
> + child_phy->routing_attr !=
> + SUBTRACTIVE_ROUTING &&
> + (child_ex->table_to_table == 0 ||
> + child_phy->routing_attr !=
> TABLE_ROUTING)) {
> sas_print_parent_topology_bug(child,
> parent_phy, child_phy);
> res = -ENODEV;
> }
> diff -ru scsi-misc-2.6/include/scsi/libsas.h
> scsi-misc-2.6.new/include/scsi/libsas.h
> --- scsi-misc-2.6/include/scsi/libsas.h 2011-08-31 08:32:22.000000000
> -0400
> +++ scsi-misc-2.6.new/include/scsi/libsas.h 2011-08-31
> 09:07:31.000000000 -0400
> @@ -144,6 +144,7 @@
> u8 num_phys;
> u8 configuring:1;
> u8 conf_route_table:1;
> + u8 table_to_table:1;
> u8 enclosure_logical_id[8];
>
> struct ex_phy *ex_phy;
> diff -ru scsi-misc-2.6/include/scsi/sas.h
> scsi-misc-2.6.new/include/scsi/sas.h
> --- scsi-misc-2.6/include/scsi/sas.h 2011-08-31 08:32:22.000000000
> -0400
> +++ scsi-misc-2.6.new/include/scsi/sas.h 2011-08-31
> 09:07:31.000000000 -0400
> @@ -341,7 +341,8 @@
>
> u8 conf_route_table:1;
> u8 configuring:1;
> - u8 _r_b:6;
> + u8 _r_b:5;
> + u8 table_to_table:1;
>
> u8 _r_c;
>
> @@ -528,7 +529,8 @@
> u8 _r_a;
> u8 num_phys;
>
> - u8 _r_b:6;
> + u8 table_to_table:1;
> + u8 _r_b:5;
> u8 configuring:1;
> u8 conf_route_table:1;
>
______________________________________________________________________
This email may contain privileged or confidential information, which should only be used for the purpose for which it was sent by Xyratex. No further rights or licenses are granted to use such information. If you are not the intended recipient of this message, please notify the sender by return and delete it. You may not use, copy, disclose or rely on the information contained in it.
Internet email is susceptible to data corruption, interception and unauthorised amendment for which Xyratex does not accept liability. While we have taken reasonable precautions to ensure that this email is free of viruses, Xyratex does not accept liability for the presence of any computer viruses in this email, nor for any losses caused as a result of viruses.
Xyratex Technology Limited (03134912), Registered in England & Wales, Registered Office, Langstone Road, Havant, Hampshire, PO9 1SA.
The Xyratex group of companies also includes, Xyratex Ltd, registered in Bermuda, Xyratex International Inc, registered in California, Xyratex (Malaysia) Sdn Bhd registered in Malaysia, Xyratex Technology (Wuxi) Co Ltd registered in The People's Republic of China and Xyratex Japan Limited registered in Japan.
______________________________________________________________________
--
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] 13+ messages in thread* RE: [PATCH] [SCSI]: Allow expander T-T attachments
2011-08-31 17:51 ` Mark Salyzyn
@ 2011-09-01 15:34 ` James Bottomley
2011-09-01 15:58 ` Mark Salyzyn
2011-09-06 16:47 ` Luben Tuikov
2011-09-01 17:25 ` Luben Tuikov
1 sibling, 2 replies; 13+ messages in thread
From: James Bottomley @ 2011-09-01 15:34 UTC (permalink / raw)
To: Mark Salyzyn; +Cc: Luben Tuikov, linux-scsi, Darrick J Wong
On Wed, 2011-08-31 at 10:51 -0700, Mark Salyzyn wrote:
> We will need James to chime in why your patch did not go in. My
> assumption is the lack of response to his question left it in limbo.
That's a good assumption. A maintainers list folder is usually sorted
by thread and by activity. I mark patches for inclusion, but if a
thread is expecting a reply I don't do anything since it comes back to
the top again once there's activity on it.
> The likely modification to your patch would be to remove all the
> unused added flags rather than staking out the territory.
Well, yes, but the question is broader than that. In theory we can't
support SAS2.0 expanders until we support all of their features. Table
to Table routing is one, but self configuration is another. If there
are no current self configuring expanders, I'm OK with just doing table
to table ... but the extra bits made me wonder.
James
> What kind of testing was involved in your patch? I have not merged
> yours in here to try it out on our enclosures. Either way, I want to
> see some movement; James any directions?
>
> Sincerely -- Mark Salyzyn
>
> -----Original Message-----
> From: Luben Tuikov [mailto:ltuikov@yahoo.com]
> Sent: Wednesday, August 31, 2011 1:43 PM
> To: Mark Salyzyn; linux-scsi@vger.kernel.org
> Cc: Darrick J Wong; James Bottomley
> Subject: Re: [PATCH] [SCSI]: Allow expander T-T attachments
>
> ----- Original Message -----
>
> > From: Mark Salyzyn <mark_salyzyn@us.xyratex.com>
> > To: linux-scsi@vger.kernel.org
> > Cc: Luben Tuikov <ltuikov@yahoo.com>; Darrick J Wong <djwong@us.ibm.com>; James Bottomley <jbottomley@parallels.com>
> > Sent: Wednesday, August 31, 2011 6:44 AM
> > Subject: Re: [PATCH] [SCSI]: Allow expander T-T attachments
> >
> > Since Luben's patch on July 28th 2011 went nowhere, as there was an
> > unanswered question regarding the future-proofing that Luben added in.
>
> Are you saying that the patch I posted,
> http://marc.info/?t=131182720200001&r=1&w=2, didn't go in
> because I didn't entertain Bottomley's unrelated question therein?
> Or are you saying that it did go in based on TECHNICAL CONTENT? I.e. it
> was buggy, or wrong, etc?
>
> > I decided to submit a simplified focussed patch we have been using for the
>
> By "simplified, focused" you mean one which doesn't define 2 out of 3 flags of
> REPORT GENERAL SMP response?
>
> > past year testing with the Xyratex enclosures (5U84, 2U24, chained with
> > *many* peered expanders with Table to Table routing) that should
> > hopefully expedite things and will be on hot standby to refactor as
> > requested (including tossing this baby and refactoring Luben's patch
> > should he be too busy, no hair off my back)
>
> Now onto technical matter:
> 1) I'd call the flag "t2t_supp" to a) fit in with the rest of the spirit of the
> code as other fields are also called "_supp", and to b) actually convey
> the meaning of "table-to-table supported. Your naming of this flag
> "table_to_table" implicitly means "table to table supported".
> 2) Please also add the debug output as my patch did. People will
> be wondering what went wrong if their domain didn't work, and the
> debug print in my original patch would tell them that.
> 3) You don't check that the parent also supports T2T. That's a bug.
> 4) Your patch logically does this: P=T && C!=S && (C is T2T || C!=T),
> which after expansion is equivalent to:
> (P==T) && ((C!=S && C!=T2T) || C==D) ==> fail
> where the outmost parens are an if (). Albeit from not being entirely correct,
> it also obfuscates what is being sought after. I chose to use the negation for
> successful check to make it more clear:
> (P==T) && (C==S || (C==T && P==T2T && C==T2T)) ==> success,
> Translates to
> "If parent phy is T, and If child phy is S OR (child is T and child and parent
> both support table to table) then we're okay, else report an error."
> Which is: T<-S is okay, and T<-T is okay if both parent and child support it.
>
> Alternatively, Bottomley could just add my patch upstream.
>
> > Please note, as I am *stuck* on Outlook as per company policy, the
> > following inline content will likely not patch clean even emailed as
> > 'Plain Text', the enclosed attached file should do the job. I have
> > Cc'd
> > all the folks that originated the files in libsas, as there was no
> > listed MAINTAINERs.
> >
> > NB: this patch (and Luben's independent patch) results in an ABI break
> > as the structures change 'shape' and thus result in a different set of
> > libsas export signatures. I have an internal patch I use that preserves
> > the structure shapes and thus the ABI; but would be considered
> > inappropriate for the pristine trees. Said alternate patch would work
> > fine for a Distribution tree where ABI concerns are an issue.
> >
> > Checkpatch.pl reports clean.
> >
> > Sincerely -- Mark Salyzyn
> >
> > Cc: Luben Tuikov <tuikov@yahoo.com>
> > Cc: Darrick J Wong <djwong@us.ibm.com>
> > Cc: James Bottomley <jbottomley@parallels.com>
> >
> > Signed-off-by: Mark Salyzyn <msalyzyn@us.xyratex.com>
> >
> > drivers/scsi/libsas/sas_expander.c | 6 +++++-
> > include/scsi/libsas.h | 1 +
> > include/scsi/sas.h | 6 ++++--
> > 3 files changed, 10 insertions(+), 3 deletions(-)
> >
> > diff -ru scsi-misc-2.6/drivers/scsi/libsas/sas_expander.c
> > scsi-misc-2.6.new/drivers/scsi/libsas/sas_expander.c
> > --- scsi-misc-2.6/drivers/scsi/libsas/sas_expander.c 2011-08-31
> > 08:32:21.000000000 -0400
> > +++ scsi-misc-2.6.new/drivers/scsi/libsas/sas_expander.c
> > 2011-08-31 09:07:31.000000000 -0400
> > @@ -331,6 +331,7 @@
> > dev->ex_dev.num_phys = min(rg->num_phys, (u8)MAX_EXPANDER_PHYS);
> > dev->ex_dev.conf_route_table = rg->conf_route_table;
> > dev->ex_dev.configuring = rg->configuring;
> > + dev->ex_dev.table_to_table = rg->table_to_table;
> > memcpy(dev->ex_dev.enclosure_logical_id,
> > rg->enclosure_logical_id, 8);
> > }
> >
> > @@ -1239,7 +1240,10 @@
> > res = -ENODEV;
> > }
> > } else if (parent_phy->routing_attr ==
> > TABLE_ROUTING &&
> > - child_phy->routing_attr !=
> > SUBTRACTIVE_ROUTING) {
> > + child_phy->routing_attr !=
> > + SUBTRACTIVE_ROUTING &&
> > + (child_ex->table_to_table == 0 ||
> > + child_phy->routing_attr !=
> > TABLE_ROUTING)) {
> > sas_print_parent_topology_bug(child,
> > parent_phy, child_phy);
> > res = -ENODEV;
> > }
> > diff -ru scsi-misc-2.6/include/scsi/libsas.h
> > scsi-misc-2.6.new/include/scsi/libsas.h
> > --- scsi-misc-2.6/include/scsi/libsas.h 2011-08-31 08:32:22.000000000
> > -0400
> > +++ scsi-misc-2.6.new/include/scsi/libsas.h 2011-08-31
> > 09:07:31.000000000 -0400
> > @@ -144,6 +144,7 @@
> > u8 num_phys;
> > u8 configuring:1;
> > u8 conf_route_table:1;
> > + u8 table_to_table:1;
> > u8 enclosure_logical_id[8];
> >
> > struct ex_phy *ex_phy;
> > diff -ru scsi-misc-2.6/include/scsi/sas.h
> > scsi-misc-2.6.new/include/scsi/sas.h
> > --- scsi-misc-2.6/include/scsi/sas.h 2011-08-31 08:32:22.000000000
> > -0400
> > +++ scsi-misc-2.6.new/include/scsi/sas.h 2011-08-31
> > 09:07:31.000000000 -0400
> > @@ -341,7 +341,8 @@
> >
> > u8 conf_route_table:1;
> > u8 configuring:1;
> > - u8 _r_b:6;
> > + u8 _r_b:5;
> > + u8 table_to_table:1;
> >
> > u8 _r_c;
> >
> > @@ -528,7 +529,8 @@
> > u8 _r_a;
> > u8 num_phys;
> >
> > - u8 _r_b:6;
> > + u8 table_to_table:1;
> > + u8 _r_b:5;
> > u8 configuring:1;
> > u8 conf_route_table:1;
> >
> ______________________________________________________________________
> This email may contain privileged or confidential information, which should only be used for the purpose for which it was sent by Xyratex. No further rights or licenses are granted to use such information. If you are not the intended recipient of this message, please notify the sender by return and delete it. You may not use, copy, disclose or rely on the information contained in it.
>
> Internet email is susceptible to data corruption, interception and unauthorised amendment for which Xyratex does not accept liability. While we have taken reasonable precautions to ensure that this email is free of viruses, Xyratex does not accept liability for the presence of any computer viruses in this email, nor for any losses caused as a result of viruses.
>
> Xyratex Technology Limited (03134912), Registered in England & Wales, Registered Office, Langstone Road, Havant, Hampshire, PO9 1SA.
>
> The Xyratex group of companies also includes, Xyratex Ltd, registered in Bermuda, Xyratex International Inc, registered in California, Xyratex (Malaysia) Sdn Bhd registered in Malaysia, Xyratex Technology (Wuxi) Co Ltd registered in The People's Republic of China and Xyratex Japan Limited registered in Japan.
> ______________________________________________________________________
>
>
> --
> 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] 13+ messages in thread* RE: [PATCH] [SCSI]: Allow expander T-T attachments
2011-09-01 15:34 ` James Bottomley
@ 2011-09-01 15:58 ` Mark Salyzyn
2011-09-06 16:47 ` Luben Tuikov
1 sibling, 0 replies; 13+ messages in thread
From: Mark Salyzyn @ 2011-09-01 15:58 UTC (permalink / raw)
To: James Bottomley; +Cc: Luben Tuikov, linux-scsi, Darrick J Wong
Ok James, my take on this (a rationalization):
I believe the SAS 2.0 enhancements are orthogonal and the expanders will
stage-in these new features.
Every change in the shape of the libsas structures creates an ABI break
for all the libsas-dependent drivers. Fine for kernel.org, expected and
barely noticed, but the ABI break affects Distributions as they will not
be able to weakly-link modules from Quarterly Update to Quarterly
Update. This will create resistance at every step of the way as SAS 2.0
code migrates in from whatever source.
I find Luben's patch suitable primarily because it has staked some
useful territory which will make the move to all the SAS 2.0 features
less costly on the Distributions. I also find that this territory, even
if Luben has or does not have plans, to be inert but future-proofed.
Knowing how to use them may create issues though for the chosen
algorithms and for that reason alone I could be convinced that we should
drop these until someone somewhere uses them in their own model (ok, I
waffle, but placing /* reserved */ after each will solve most of the
worry about misuse).
I would expect that the enhancements to deal with self-configuring
expanders (which Xyratex does not have ... yet) to be an orthogonal
effort (minus the staked out territory) and along with other issues and
deficiencies more complicated to merge in.
I would appeal that you, and all other stakeholders, be willing to take
Luben's patch unmodified (or at least with /* reserved */ comments) with
the understanding that someone somewhere is going to take on the other
enhancements for 2.0 as they hit *real* hardware. And if this one is
taken (to at least the pending tree), we will make sure it is tested and
get back to the list if we find borkenness(sic).
Approved: <msalyzyn@us.xyratex.com>
Sincerely -- Mark Salyzyn
ObligatoryDisclaimer: Ignore the Xyratex tacked-on disclaimer.
-----Original Message-----
From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com]
Sent: Thursday, September 01, 2011 11:34 AM
To: Mark Salyzyn
Cc: Luben Tuikov; linux-scsi@vger.kernel.org; Darrick J Wong
Subject: RE: [PATCH] [SCSI]: Allow expander T-T attachments
On Wed, 2011-08-31 at 10:51 -0700, Mark Salyzyn wrote:
> We will need James to chime in why your patch did not go in. My
> assumption is the lack of response to his question left it in limbo.
That's a good assumption. A maintainers list folder is usually sorted
by thread and by activity. I mark patches for inclusion, but if a
thread is expecting a reply I don't do anything since it comes back to
the top again once there's activity on it.
> The likely modification to your patch would be to remove all the
> unused added flags rather than staking out the territory.
Well, yes, but the question is broader than that. In theory we can't
support SAS2.0 expanders until we support all of their features. Table
to Table routing is one, but self configuration is another. If there
are no current self configuring expanders, I'm OK with just doing table
to table ... but the extra bits made me wonder.
James
> What kind of testing was involved in your patch? I have not merged
> yours in here to try it out on our enclosures. Either way, I want to
> see some movement; James any directions?
>
> Sincerely -- Mark Salyzyn
>
> -----Original Message-----
> From: Luben Tuikov [mailto:ltuikov@yahoo.com]
> Sent: Wednesday, August 31, 2011 1:43 PM
> To: Mark Salyzyn; linux-scsi@vger.kernel.org
> Cc: Darrick J Wong; James Bottomley
> Subject: Re: [PATCH] [SCSI]: Allow expander T-T attachments
>
> ----- Original Message -----
>
> > From: Mark Salyzyn <mark_salyzyn@us.xyratex.com>
> > To: linux-scsi@vger.kernel.org
> > Cc: Luben Tuikov <ltuikov@yahoo.com>; Darrick J Wong
<djwong@us.ibm.com>; James Bottomley <jbottomley@parallels.com>
> > Sent: Wednesday, August 31, 2011 6:44 AM
> > Subject: Re: [PATCH] [SCSI]: Allow expander T-T attachments
> >
> > Since Luben's patch on July 28th 2011 went nowhere, as there was an
> > unanswered question regarding the future-proofing that Luben added
in.
>
> Are you saying that the patch I posted,
> http://marc.info/?t=131182720200001&r=1&w=2, didn't go in
> because I didn't entertain Bottomley's unrelated question therein?
> Or are you saying that it did go in based on TECHNICAL CONTENT? I.e.
it
> was buggy, or wrong, etc?
>
> > I decided to submit a simplified focussed patch we have been using
for the
>
> By "simplified, focused" you mean one which doesn't define 2 out of 3
flags of
> REPORT GENERAL SMP response?
>
> > past year testing with the Xyratex enclosures (5U84, 2U24, chained
with
> > *many* peered expanders with Table to Table routing) that should
> > hopefully expedite things and will be on hot standby to refactor as
> > requested (including tossing this baby and refactoring Luben's patch
> > should he be too busy, no hair off my back)
>
> Now onto technical matter:
> 1) I'd call the flag "t2t_supp" to a) fit in with the rest of the
spirit of the
> code as other fields are also called "_supp", and to b) actually
convey
> the meaning of "table-to-table supported. Your naming of this flag
> "table_to_table" implicitly means "table to table supported".
> 2) Please also add the debug output as my patch did. People will
> be wondering what went wrong if their domain didn't work, and the
> debug print in my original patch would tell them that.
> 3) You don't check that the parent also supports T2T. That's a bug.
> 4) Your patch logically does this: P=T && C!=S && (C is T2T || C!=T),
> which after expansion is equivalent to:
> (P==T) && ((C!=S && C!=T2T) || C==D) ==> fail
> where the outmost parens are an if (). Albeit from not being entirely
correct,
> it also obfuscates what is being sought after. I chose to use the
negation for
> successful check to make it more clear:
> (P==T) && (C==S || (C==T && P==T2T && C==T2T)) ==> success,
> Translates to
> "If parent phy is T, and If child phy is S OR (child is T and child
and parent
> both support table to table) then we're okay, else report an error."
> Which is: T<-S is okay, and T<-T is okay if both parent and child
support it.
>
> Alternatively, Bottomley could just add my patch upstream.
>
> > Please note, as I am *stuck* on Outlook as per company policy, the
> > following inline content will likely not patch clean even emailed as
> > 'Plain Text', the enclosed attached file should do the job. I have
> > Cc'd
> > all the folks that originated the files in libsas, as there was no
> > listed MAINTAINERs.
> >
> > NB: this patch (and Luben's independent patch) results in an ABI
break
> > as the structures change 'shape' and thus result in a different set
of
> > libsas export signatures. I have an internal patch I use that
preserves
> > the structure shapes and thus the ABI; but would be considered
> > inappropriate for the pristine trees. Said alternate patch would
work
> > fine for a Distribution tree where ABI concerns are an issue.
> >
> > Checkpatch.pl reports clean.
> >
> > Sincerely -- Mark Salyzyn
> >
> > Cc: Luben Tuikov <tuikov@yahoo.com>
> > Cc: Darrick J Wong <djwong@us.ibm.com>
> > Cc: James Bottomley <jbottomley@parallels.com>
> >
> > Signed-off-by: Mark Salyzyn <msalyzyn@us.xyratex.com>
> >
> > drivers/scsi/libsas/sas_expander.c | 6 +++++-
> > include/scsi/libsas.h | 1 +
> > include/scsi/sas.h | 6 ++++--
> > 3 files changed, 10 insertions(+), 3 deletions(-)
> >
> > diff -ru scsi-misc-2.6/drivers/scsi/libsas/sas_expander.c
> > scsi-misc-2.6.new/drivers/scsi/libsas/sas_expander.c
> > --- scsi-misc-2.6/drivers/scsi/libsas/sas_expander.c 2011-08-31
> > 08:32:21.000000000 -0400
> > +++ scsi-misc-2.6.new/drivers/scsi/libsas/sas_expander.c
> > 2011-08-31 09:07:31.000000000 -0400
> > @@ -331,6 +331,7 @@
> > dev->ex_dev.num_phys = min(rg->num_phys, (u8)MAX_EXPANDER_PHYS);
> > dev->ex_dev.conf_route_table = rg->conf_route_table;
> > dev->ex_dev.configuring = rg->configuring;
> > + dev->ex_dev.table_to_table = rg->table_to_table;
> > memcpy(dev->ex_dev.enclosure_logical_id,
> > rg->enclosure_logical_id, 8);
> > }
> >
> > @@ -1239,7 +1240,10 @@
> > res = -ENODEV;
> > }
> > } else if (parent_phy->routing_attr ==
> > TABLE_ROUTING &&
> > - child_phy->routing_attr !=
> > SUBTRACTIVE_ROUTING) {
> > + child_phy->routing_attr !=
> > + SUBTRACTIVE_ROUTING &&
> > + (child_ex->table_to_table == 0 ||
> > + child_phy->routing_attr !=
> > TABLE_ROUTING)) {
> > sas_print_parent_topology_bug(child,
> > parent_phy, child_phy);
> > res = -ENODEV;
> > }
> > diff -ru scsi-misc-2.6/include/scsi/libsas.h
> > scsi-misc-2.6.new/include/scsi/libsas.h
> > --- scsi-misc-2.6/include/scsi/libsas.h 2011-08-31
08:32:22.000000000
> > -0400
> > +++ scsi-misc-2.6.new/include/scsi/libsas.h 2011-08-31
> > 09:07:31.000000000 -0400
> > @@ -144,6 +144,7 @@
> > u8 num_phys;
> > u8 configuring:1;
> > u8 conf_route_table:1;
> > + u8 table_to_table:1;
> > u8 enclosure_logical_id[8];
> >
> > struct ex_phy *ex_phy;
> > diff -ru scsi-misc-2.6/include/scsi/sas.h
> > scsi-misc-2.6.new/include/scsi/sas.h
> > --- scsi-misc-2.6/include/scsi/sas.h 2011-08-31
08:32:22.000000000
> > -0400
> > +++ scsi-misc-2.6.new/include/scsi/sas.h 2011-08-31
> > 09:07:31.000000000 -0400
> > @@ -341,7 +341,8 @@
> >
> > u8 conf_route_table:1;
> > u8 configuring:1;
> > - u8 _r_b:6;
> > + u8 _r_b:5;
> > + u8 table_to_table:1;
> >
> > u8 _r_c;
> >
> > @@ -528,7 +529,8 @@
> > u8 _r_a;
> > u8 num_phys;
> >
> > - u8 _r_b:6;
> > + u8 table_to_table:1;
> > + u8 _r_b:5;
> > u8 configuring:1;
> > u8 conf_route_table:1;
> >
______________________________________________________________________
This email may contain privileged or confidential information, which should only be used for the purpose for which it was sent by Xyratex. No further rights or licenses are granted to use such information. If you are not the intended recipient of this message, please notify the sender by return and delete it. You may not use, copy, disclose or rely on the information contained in it.
Internet email is susceptible to data corruption, interception and unauthorised amendment for which Xyratex does not accept liability. While we have taken reasonable precautions to ensure that this email is free of viruses, Xyratex does not accept liability for the presence of any computer viruses in this email, nor for any losses caused as a result of viruses.
Xyratex Technology Limited (03134912), Registered in England & Wales, Registered Office, Langstone Road, Havant, Hampshire, PO9 1SA.
The Xyratex group of companies also includes, Xyratex Ltd, registered in Bermuda, Xyratex International Inc, registered in California, Xyratex (Malaysia) Sdn Bhd registered in Malaysia, Xyratex Technology (Wuxi) Co Ltd registered in The People's Republic of China and Xyratex Japan Limited registered in Japan.
______________________________________________________________________
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] [SCSI]: Allow expander T-T attachments
2011-09-01 15:34 ` James Bottomley
2011-09-01 15:58 ` Mark Salyzyn
@ 2011-09-06 16:47 ` Luben Tuikov
1 sibling, 0 replies; 13+ messages in thread
From: Luben Tuikov @ 2011-09-06 16:47 UTC (permalink / raw)
To: James Bottomley, Mark Salyzyn; +Cc: linux-scsi, Darrick J Wong
----- Original Message -----
>> We will need James to chime in why your patch did not go in. My
>> assumption is the lack of response to his question left it in limbo.
>
> That's a good assumption. A maintainers list folder is usually sorted
> by thread and by activity. I mark patches for inclusion, but if a
> thread is expecting a reply I don't do anything since it comes back to
> the top again once there's activity on it.
This kernel business shouldn't be about you. It should be about
making the kernel better today than it is tomorrow, so that people can
use it and can use it with more devices, without bugs.
>> The likely modification to your patch would be to remove all the
>> unused added flags rather than staking out the territory.
>
> Well, yes, but the question is broader than that. In theory we can't
> support SAS2.0 expanders until we support all of their features.
That's strictly _your_ theory and what it does, it excludes functionality
from the kernel instead of enabling it. When two vendors send you
a patch that enables the same functionality and ask for it to be
included into the kernel, because people are using this functionality,
you should listen and include it.
> Table
> to Table routing is one, but self configuration is another.
That's a very uninformed statement. It shows your utter lack of familiarity
of the SAS protocol.
> If there
> are no current self configuring expanders, I'm OK with just doing table
> to table ... but the extra bits made me wonder.
Yes, you should simply include the patch.
>
> James
>
>
>> What kind of testing was involved in your patch? I have not merged
>> yours in here to try it out on our enclosures. Either way, I want to
>> see some movement; James any directions?
>>
>> Sincerely -- Mark Salyzyn
>>
>> -----Original Message-----
>> From: Luben Tuikov [mailto:ltuikov@yahoo.com]
>> Sent: Wednesday, August 31, 2011 1:43 PM
>> To: Mark Salyzyn; linux-scsi@vger.kernel.org
>> Cc: Darrick J Wong; James Bottomley
>> Subject: Re: [PATCH] [SCSI]: Allow expander T-T attachments
>>
>> ----- Original Message -----
>>
>> > From: Mark Salyzyn <mark_salyzyn@us.xyratex.com>
>> > To: linux-scsi@vger.kernel.org
>> > Cc: Luben Tuikov <ltuikov@yahoo.com>; Darrick J Wong
> <djwong@us.ibm.com>; James Bottomley <jbottomley@parallels.com>
>> > Sent: Wednesday, August 31, 2011 6:44 AM
>> > Subject: Re: [PATCH] [SCSI]: Allow expander T-T attachments
>> >
>> > Since Luben's patch on July 28th 2011 went nowhere, as there was
> an
>> > unanswered question regarding the future-proofing that Luben added in.
>>
>> Are you saying that the patch I posted,
>> http://marc.info/?t=131182720200001&r=1&w=2, didn't go in
>> because I didn't entertain Bottomley's unrelated question therein?
>> Or are you saying that it did go in based on TECHNICAL CONTENT? I.e. it
>> was buggy, or wrong, etc?
>>
>> > I decided to submit a simplified focussed patch we have been using for
> the
>>
>> By "simplified, focused" you mean one which doesn't define 2
> out of 3 flags of
>> REPORT GENERAL SMP response?
>>
>> > past year testing with the Xyratex enclosures (5U84, 2U24, chained
> with
>> > *many* peered expanders with Table to Table routing) that should
>> > hopefully expedite things and will be on hot standby to refactor as
>> > requested (including tossing this baby and refactoring Luben's
> patch
>> > should he be too busy, no hair off my back)
>>
>> Now onto technical matter:
>> 1) I'd call the flag "t2t_supp" to a) fit in with the rest of
> the spirit of the
>> code as other fields are also called "_supp", and to b) actually
> convey
>> the meaning of "table-to-table supported. Your naming of this flag
>> "table_to_table" implicitly means "table to table
> supported".
>> 2) Please also add the debug output as my patch did. People will
>> be wondering what went wrong if their domain didn't work, and the
>> debug print in my original patch would tell them that.
>> 3) You don't check that the parent also supports T2T. That's a bug.
>> 4) Your patch logically does this: P=T && C!=S && (C is T2T
> || C!=T),
>> which after expansion is equivalent to:
>> (P==T) && ((C!=S && C!=T2T) || C==D) ==> fail
>> where the outmost parens are an if (). Albeit from not being entirely
> correct,
>> it also obfuscates what is being sought after. I chose to use the negation
> for
>> successful check to make it more clear:
>> (P==T) && (C==S || (C==T && P==T2T && C==T2T))
> ==> success,
>> Translates to
>> "If parent phy is T, and If child phy is S OR (child is T and child
> and parent
>> both support table to table) then we're okay, else report an
> error."
>> Which is: T<-S is okay, and T<-T is okay if both parent and child
> support it.
>>
>> Alternatively, Bottomley could just add my patch upstream.
>>
>> > Please note, as I am *stuck* on Outlook as per company policy, the
>> > following inline content will likely not patch clean even emailed as
>> > 'Plain Text', the enclosed attached file should do the job. I
> have
>> > Cc'd
>> > all the folks that originated the files in libsas, as there was no
>> > listed MAINTAINERs.
>> >
>> > NB: this patch (and Luben's independent patch) results in an ABI
> break
>> > as the structures change 'shape' and thus result in a
> different set of
>> > libsas export signatures. I have an internal patch I use that
> preserves
>> > the structure shapes and thus the ABI; but would be considered
>> > inappropriate for the pristine trees. Said alternate patch would work
>> > fine for a Distribution tree where ABI concerns are an issue.
>> >
>> > Checkpatch.pl reports clean.
>> >
>> > Sincerely -- Mark Salyzyn
>> >
>> > Cc: Luben Tuikov <tuikov@yahoo.com>
>> > Cc: Darrick J Wong <djwong@us.ibm.com>
>> > Cc: James Bottomley <jbottomley@parallels.com>
>> >
>> > Signed-off-by: Mark Salyzyn <msalyzyn@us.xyratex.com>
>> >
>> > drivers/scsi/libsas/sas_expander.c | 6 +++++-
>> > include/scsi/libsas.h | 1 +
>> > include/scsi/sas.h | 6 ++++--
>> > 3 files changed, 10 insertions(+), 3 deletions(-)
>> >
>> > diff -ru scsi-misc-2.6/drivers/scsi/libsas/sas_expander.c
>> > scsi-misc-2.6.new/drivers/scsi/libsas/sas_expander.c
>> > --- scsi-misc-2.6/drivers/scsi/libsas/sas_expander.c 2011-08-31
>> > 08:32:21.000000000 -0400
>> > +++ scsi-misc-2.6.new/drivers/scsi/libsas/sas_expander.c
>> > 2011-08-31 09:07:31.000000000 -0400
>> > @@ -331,6 +331,7 @@
>> > dev->ex_dev.num_phys = min(rg->num_phys,
> (u8)MAX_EXPANDER_PHYS);
>> > dev->ex_dev.conf_route_table = rg->conf_route_table;
>> > dev->ex_dev.configuring = rg->configuring;
>> > + dev->ex_dev.table_to_table = rg->table_to_table;
>> > memcpy(dev->ex_dev.enclosure_logical_id,
>> > rg->enclosure_logical_id, 8);
>> > }
>> >
>> > @@ -1239,7 +1240,10 @@
>> > res = -ENODEV;
>> > }
>> > } else if (parent_phy->routing_attr ==
>> > TABLE_ROUTING &&
>> > - child_phy->routing_attr !=
>> > SUBTRACTIVE_ROUTING) {
>> > + child_phy->routing_attr !=
>> > + SUBTRACTIVE_ROUTING &&
>> > + (child_ex->table_to_table == 0 ||
>> > + child_phy->routing_attr !=
>> > TABLE_ROUTING)) {
>> > sas_print_parent_topology_bug(child,
>> > parent_phy, child_phy);
>> > res = -ENODEV;
>> > }
>> > diff -ru scsi-misc-2.6/include/scsi/libsas.h
>> > scsi-misc-2.6.new/include/scsi/libsas.h
>> > --- scsi-misc-2.6/include/scsi/libsas.h 2011-08-31
> 08:32:22.000000000
>> > -0400
>> > +++ scsi-misc-2.6.new/include/scsi/libsas.h 2011-08-31
>> > 09:07:31.000000000 -0400
>> > @@ -144,6 +144,7 @@
>> > u8 num_phys;
>> > u8 configuring:1;
>> > u8 conf_route_table:1;
>> > + u8 table_to_table:1;
>> > u8 enclosure_logical_id[8];
>> >
>> > struct ex_phy *ex_phy;
>> > diff -ru scsi-misc-2.6/include/scsi/sas.h
>> > scsi-misc-2.6.new/include/scsi/sas.h
>> > --- scsi-misc-2.6/include/scsi/sas.h 2011-08-31 08:32:22.000000000
>> > -0400
>> > +++ scsi-misc-2.6.new/include/scsi/sas.h 2011-08-31
>> > 09:07:31.000000000 -0400
>> > @@ -341,7 +341,8 @@
>> >
>> > u8 conf_route_table:1;
>> > u8 configuring:1;
>> > - u8 _r_b:6;
>> > + u8 _r_b:5;
>> > + u8 table_to_table:1;
>> >
>> > u8 _r_c;
>> >
>> > @@ -528,7 +529,8 @@
>> > u8 _r_a;
>> > u8 num_phys;
>> >
>> > - u8 _r_b:6;
>> > + u8 table_to_table:1;
>> > + u8 _r_b:5;
>> > u8 configuring:1;
>> > u8 conf_route_table:1;
>> >
>> ______________________________________________________________________
>> This email may contain privileged or confidential information, which should
> only be used for the purpose for which it was sent by Xyratex. No further rights
> or licenses are granted to use such information. If you are not the intended
> recipient of this message, please notify the sender by return and delete it. You
> may not use, copy, disclose or rely on the information contained in it.
>>
>> Internet email is susceptible to data corruption, interception and
> unauthorised amendment for which Xyratex does not accept liability. While we
> have taken reasonable precautions to ensure that this email is free of viruses,
> Xyratex does not accept liability for the presence of any computer viruses in
> this email, nor for any losses caused as a result of viruses.
>>
>> Xyratex Technology Limited (03134912), Registered in England & Wales,
> Registered Office, Langstone Road, Havant, Hampshire, PO9 1SA.
>>
>> The Xyratex group of companies also includes, Xyratex Ltd, registered in
> Bermuda, Xyratex International Inc, registered in California, Xyratex (Malaysia)
> Sdn Bhd registered in Malaysia, Xyratex Technology (Wuxi) Co Ltd registered in
> The People's Republic of China and Xyratex Japan Limited registered in
> Japan.
>> ______________________________________________________________________
>>
>>
>> --
>> 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
>
--
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] 13+ messages in thread
* Re: [PATCH] [SCSI]: Allow expander T-T attachments
2011-08-31 17:51 ` Mark Salyzyn
2011-09-01 15:34 ` James Bottomley
@ 2011-09-01 17:25 ` Luben Tuikov
1 sibling, 0 replies; 13+ messages in thread
From: Luben Tuikov @ 2011-09-01 17:25 UTC (permalink / raw)
To: Mark Salyzyn, linux-scsi; +Cc: Darrick J Wong, James Bottomley
It was tested on a bunch of self-configuring, T2T supporting expanders, connected as a binary tree with a SAS controller at the root of the tree, forming more than 2 levels of branching.
----- Original Message -----
> From: Mark Salyzyn <mark_salyzyn@us.xyratex.com>
> To: Luben Tuikov <ltuikov@yahoo.com>; linux-scsi@vger.kernel.org
> Cc: Darrick J Wong <djwong@us.ibm.com>; James Bottomley <jbottomley@parallels.com>
> Sent: Wednesday, August 31, 2011 10:51 AM
> Subject: RE: [PATCH] [SCSI]: Allow expander T-T attachments
>
> We will need James to chime in why your patch did not go in. My assumption is
> the lack of response to his question left it in limbo.
>
> The likely modification to your patch would be to remove all the unused added
> flags rather than staking out the territory.
>
> What kind of testing was involved in your patch? I have not merged yours in here
> to try it out on our enclosures. Either way, I want to see some movement; James
> any directions?
>
> Sincerely -- Mark Salyzyn
>
> -----Original Message-----
> From: Luben Tuikov [mailto:ltuikov@yahoo.com]
> Sent: Wednesday, August 31, 2011 1:43 PM
> To: Mark Salyzyn; linux-scsi@vger.kernel.org
> Cc: Darrick J Wong; James Bottomley
> Subject: Re: [PATCH] [SCSI]: Allow expander T-T attachments
>
> ----- Original Message -----
>
>> From: Mark Salyzyn <mark_salyzyn@us.xyratex.com>
>> To: linux-scsi@vger.kernel.org
>> Cc: Luben Tuikov <ltuikov@yahoo.com>; Darrick J Wong
> <djwong@us.ibm.com>; James Bottomley <jbottomley@parallels.com>
>> Sent: Wednesday, August 31, 2011 6:44 AM
>> Subject: Re: [PATCH] [SCSI]: Allow expander T-T attachments
>>
>> Since Luben's patch on July 28th 2011 went nowhere, as there was an
>> unanswered question regarding the future-proofing that Luben added in.
>
> Are you saying that the patch I posted,
> http://marc.info/?t=131182720200001&r=1&w=2, didn't go in
> because I didn't entertain Bottomley's unrelated question therein?
> Or are you saying that it did go in based on TECHNICAL CONTENT? I.e. it
> was buggy, or wrong, etc?
>
>> I decided to submit a simplified focussed patch we have been using for the
>
> By "simplified, focused" you mean one which doesn't define 2 out
> of 3 flags of
> REPORT GENERAL SMP response?
>
>> past year testing with the Xyratex enclosures (5U84, 2U24, chained with
>> *many* peered expanders with Table to Table routing) that should
>> hopefully expedite things and will be on hot standby to refactor as
>> requested (including tossing this baby and refactoring Luben's patch
>> should he be too busy, no hair off my back)
>
> Now onto technical matter:
> 1) I'd call the flag "t2t_supp" to a) fit in with the rest of the
> spirit of the
> code as other fields are also called "_supp", and to b) actually
> convey
> the meaning of "table-to-table supported. Your naming of this flag
> "table_to_table" implicitly means "table to table
> supported".
> 2) Please also add the debug output as my patch did. People will
> be wondering what went wrong if their domain didn't work, and the
> debug print in my original patch would tell them that.
> 3) You don't check that the parent also supports T2T. That's a bug.
> 4) Your patch logically does this: P=T && C!=S && (C is T2T ||
> C!=T),
> which after expansion is equivalent to:
> (P==T) && ((C!=S && C!=T2T) || C==D) ==> fail
> where the outmost parens are an if (). Albeit from not being entirely correct,
> it also obfuscates what is being sought after. I chose to use the negation for
> successful check to make it more clear:
> (P==T) && (C==S || (C==T && P==T2T && C==T2T)) ==>
> success,
> Translates to
> "If parent phy is T, and If child phy is S OR (child is T and child and
> parent
> both support table to table) then we're okay, else report an error."
> Which is: T<-S is okay, and T<-T is okay if both parent and child support
> it.
>
> Alternatively, Bottomley could just add my patch upstream.
>
>> Please note, as I am *stuck* on Outlook as per company policy, the
>> following inline content will likely not patch clean even emailed as
>> 'Plain Text', the enclosed attached file should do the job. I have
>> Cc'd
>> all the folks that originated the files in libsas, as there was no
>> listed MAINTAINERs.
>>
>> NB: this patch (and Luben's independent patch) results in an ABI break
>> as the structures change 'shape' and thus result in a different set
> of
>> libsas export signatures. I have an internal patch I use that preserves
>> the structure shapes and thus the ABI; but would be considered
>> inappropriate for the pristine trees. Said alternate patch would work
>> fine for a Distribution tree where ABI concerns are an issue.
>>
>> Checkpatch.pl reports clean.
>>
>> Sincerely -- Mark Salyzyn
>>
>> Cc: Luben Tuikov <tuikov@yahoo.com>
>> Cc: Darrick J Wong <djwong@us.ibm.com>
>> Cc: James Bottomley <jbottomley@parallels.com>
>>
>> Signed-off-by: Mark Salyzyn <msalyzyn@us.xyratex.com>
>>
>> drivers/scsi/libsas/sas_expander.c | 6 +++++-
>> include/scsi/libsas.h | 1 +
>> include/scsi/sas.h | 6 ++++--
>> 3 files changed, 10 insertions(+), 3 deletions(-)
>>
>> diff -ru scsi-misc-2.6/drivers/scsi/libsas/sas_expander.c
>> scsi-misc-2.6.new/drivers/scsi/libsas/sas_expander.c
>> --- scsi-misc-2.6/drivers/scsi/libsas/sas_expander.c 2011-08-31
>> 08:32:21.000000000 -0400
>> +++ scsi-misc-2.6.new/drivers/scsi/libsas/sas_expander.c
>> 2011-08-31 09:07:31.000000000 -0400
>> @@ -331,6 +331,7 @@
>> dev->ex_dev.num_phys = min(rg->num_phys, (u8)MAX_EXPANDER_PHYS);
>> dev->ex_dev.conf_route_table = rg->conf_route_table;
>> dev->ex_dev.configuring = rg->configuring;
>> + dev->ex_dev.table_to_table = rg->table_to_table;
>> memcpy(dev->ex_dev.enclosure_logical_id,
>> rg->enclosure_logical_id, 8);
>> }
>>
>> @@ -1239,7 +1240,10 @@
>> res = -ENODEV;
>> }
>> } else if (parent_phy->routing_attr ==
>> TABLE_ROUTING &&
>> - child_phy->routing_attr !=
>> SUBTRACTIVE_ROUTING) {
>> + child_phy->routing_attr !=
>> + SUBTRACTIVE_ROUTING &&
>> + (child_ex->table_to_table == 0 ||
>> + child_phy->routing_attr !=
>> TABLE_ROUTING)) {
>> sas_print_parent_topology_bug(child,
>> parent_phy, child_phy);
>> res = -ENODEV;
>> }
>> diff -ru scsi-misc-2.6/include/scsi/libsas.h
>> scsi-misc-2.6.new/include/scsi/libsas.h
>> --- scsi-misc-2.6/include/scsi/libsas.h 2011-08-31 08:32:22.000000000
>> -0400
>> +++ scsi-misc-2.6.new/include/scsi/libsas.h 2011-08-31
>> 09:07:31.000000000 -0400
>> @@ -144,6 +144,7 @@
>> u8 num_phys;
>> u8 configuring:1;
>> u8 conf_route_table:1;
>> + u8 table_to_table:1;
>> u8 enclosure_logical_id[8];
>>
>> struct ex_phy *ex_phy;
>> diff -ru scsi-misc-2.6/include/scsi/sas.h
>> scsi-misc-2.6.new/include/scsi/sas.h
>> --- scsi-misc-2.6/include/scsi/sas.h 2011-08-31 08:32:22.000000000
>> -0400
>> +++ scsi-misc-2.6.new/include/scsi/sas.h 2011-08-31
>> 09:07:31.000000000 -0400
>> @@ -341,7 +341,8 @@
>>
>> u8 conf_route_table:1;
>> u8 configuring:1;
>> - u8 _r_b:6;
>> + u8 _r_b:5;
>> + u8 table_to_table:1;
>>
>> u8 _r_c;
>>
>> @@ -528,7 +529,8 @@
>> u8 _r_a;
>> u8 num_phys;
>>
>> - u8 _r_b:6;
>> + u8 table_to_table:1;
>> + u8 _r_b:5;
>> u8 configuring:1;
>> u8 conf_route_table:1;
>>
> ______________________________________________________________________
> This email may contain privileged or confidential information, which should only
> be used for the purpose for which it was sent by Xyratex. No further rights or
> licenses are granted to use such information. If you are not the intended
> recipient of this message, please notify the sender by return and delete it. You
> may not use, copy, disclose or rely on the information contained in it.
>
> Internet email is susceptible to data corruption, interception and unauthorised
> amendment for which Xyratex does not accept liability. While we have taken
> reasonable precautions to ensure that this email is free of viruses, Xyratex
> does not accept liability for the presence of any computer viruses in this
> email, nor for any losses caused as a result of viruses.
>
> Xyratex Technology Limited (03134912), Registered in England & Wales,
> Registered Office, Langstone Road, Havant, Hampshire, PO9 1SA.
>
> The Xyratex group of companies also includes, Xyratex Ltd, registered in
> Bermuda, Xyratex International Inc, registered in California, Xyratex (Malaysia)
> Sdn Bhd registered in Malaysia, Xyratex Technology (Wuxi) Co Ltd registered in
> The People's Republic of China and Xyratex Japan Limited registered in
> Japan.
> ______________________________________________________________________
>
--
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] 13+ messages in thread
* [PATCH] [SCSI]: libsas failure to revalidate domain for anything but the first expander child.
2011-08-31 13:44 ` [PATCH] [SCSI]: Allow expander T-T attachments Mark Salyzyn
2011-08-31 17:43 ` Luben Tuikov
@ 2011-09-01 13:11 ` Mark Salyzyn
2011-09-01 17:18 ` Luben Tuikov
2011-09-07 19:26 ` Dan Williams
1 sibling, 2 replies; 13+ messages in thread
From: Mark Salyzyn @ 2011-09-01 13:11 UTC (permalink / raw)
To: linux-scsi; +Cc: Luben Tuikov, Darrick J Wong, James Bottomley
[-- Attachment #1: Type: text/plain, Size: 3389 bytes --]
In an enclosure model where there are chaining expanders to a large body
of storage, it was discovered that libsas, responding to a broadcast
event change, would only revalidate the domain of first child expander
in the list.
The issue is that the pointer value to the discovered source device was
used to break out of the loop, rather than the content of the pointer.
This still remains non-compliant as the revalidate domain code is
supposed to loop through all child expanders, and not stop at the first
one it finds that reports a change count. However, the design of this
routine does not allow multiple device discoveries and that would be a
more complicated set of patches reserved for another day. We are fixing
the glaring bug rather than refactoring the code.
Please note, as I am *stuck* on Outlook as per company policy, the
following inline content will likely not patch clean even emailed as
'Plain Text', the enclosed attached file should do the job. I have Cc'd
all the folks that originated the files in libsas, as there was no
listed MAINTAINERs.
Checkpatch.pl reports clean. Patch applies cleanly to a WIDE variety of
kernels up to latest.
Sincerely -- Mark Salyzyn
Cc: Luben Tuikov <tuikov@yahoo.com>
Cc: Darrick J Wong <djwong@us.ibm.com>
Cc: James Bottomley <jbottomley@parallels.com>
Signed-off-by: Mark Salyzyn <msalyzyn@us.xyratex.com>
sas_expander.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff -ru scsi-misc-2.6/drivers/scsi/libsas/sas_expander.c
scsi-misc-2.6.new/drivers/scsi/libsas/sas_expander.c
--- scsi-misc-2.6/drivers/scsi/libsas/sas_expander.c 2011-08-31
08:32:21.000000000 -0400
+++ scsi-misc-2.6.new/drivers/scsi/libsas/sas_expander.c
2011-09-01 08:57:55.000000000 -0400
@@ -1721,7 +1721,7 @@
list_for_each_entry(ch, &ex->children, siblings) {
if (ch->dev_type == EDGE_DEV || ch->dev_type ==
FANOUT_DEV) {
res = sas_find_bcast_dev(ch, src_dev);
- if (src_dev)
+ if (*src_dev)
return res;
}
}
______________________________________________________________________
This email may contain privileged or confidential information, which should only be used for the purpose for which it was sent by Xyratex. No further rights or licenses are granted to use such information. If you are not the intended recipient of this message, please notify the sender by return and delete it. You may not use, copy, disclose or rely on the information contained in it.
Internet email is susceptible to data corruption, interception and unauthorised amendment for which Xyratex does not accept liability. While we have taken reasonable precautions to ensure that this email is free of viruses, Xyratex does not accept liability for the presence of any computer viruses in this email, nor for any losses caused as a result of viruses.
Xyratex Technology Limited (03134912), Registered in England & Wales, Registered Office, Langstone Road, Havant, Hampshire, PO9 1SA.
The Xyratex group of companies also includes, Xyratex Ltd, registered in Bermuda, Xyratex International Inc, registered in California, Xyratex (Malaysia) Sdn Bhd registered in Malaysia, Xyratex Technology (Wuxi) Co Ltd registered in The People's Republic of China and Xyratex Japan Limited registered in Japan.
______________________________________________________________________
\r
[-- Attachment #2: expander_bug.patch --]
[-- Type: application/octet-stream, Size: 789 bytes --]
Cc: Luben Tuikov <tuikov@yahoo.com>
Cc: Darrick J Wong <djwong@us.ibm.com>
Cc: James Bottomley <jbottomley@parallels.com>
Signed-off-by: Mark Salyzyn <msalyzyn@us.xyratex.com>
sas_expander.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff -ru scsi-misc-2.6/drivers/scsi/libsas/sas_expander.c scsi-misc-2.6.new/drivers/scsi/libsas/sas_expander.c
--- scsi-misc-2.6/drivers/scsi/libsas/sas_expander.c 2011-08-31 08:32:21.000000000 -0400
+++ scsi-misc-2.6.new/drivers/scsi/libsas/sas_expander.c 2011-09-01 08:57:55.000000000 -0400
@@ -1721,7 +1721,7 @@
list_for_each_entry(ch, &ex->children, siblings) {
if (ch->dev_type == EDGE_DEV || ch->dev_type == FANOUT_DEV) {
res = sas_find_bcast_dev(ch, src_dev);
- if (src_dev)
+ if (*src_dev)
return res;
}
}
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] [SCSI]: libsas failure to revalidate domain for anything but the first expander child.
2011-09-01 13:11 ` [PATCH] [SCSI]: libsas failure to revalidate domain for anything but the first expander child Mark Salyzyn
@ 2011-09-01 17:18 ` Luben Tuikov
2011-09-06 14:56 ` Mark Salyzyn
2011-09-07 19:26 ` Dan Williams
1 sibling, 1 reply; 13+ messages in thread
From: Luben Tuikov @ 2011-09-01 17:18 UTC (permalink / raw)
To: Mark Salyzyn, linux-scsi; +Cc: Darrick J Wong, James Bottomley
----- Original Message -----
> In an enclosure model where there are chaining expanders to a large body
> of storage, it was discovered that libsas, responding to a broadcast
> event change, would only revalidate the domain of first child expander
> in the list.
>
> The issue is that the pointer value to the discovered source device was
> used to break out of the loop, rather than the content of the pointer.
>
> This still remains non-compliant as the revalidate domain code is
> supposed to loop through all child expanders, and not stop at the first
> one it finds that reports a change count. However, the design of this
> routine does not allow multiple device discoveries and that would be a
> more complicated set of patches reserved for another day. We are fixing
> the glaring bug rather than refactoring the code.
Obviously I've tested this both when I was at Adaptec and at Vitesse. I'd connect
7-8 expanders, run iogen with 1000 threads to say 30-40 disks, and then unplug
the port between a level 1 and 2 expander and the I/O would quiesce, iogen would
report a subset of the disks missing, and then when the port was reestablished,
I/O would restart. However I'm not sure that Bottomley tested this scenario after
changing my code off-line before submitting it into the Linux kernel.
Now a few notes to mention: Your patch patches a function called
sas_find_bcast_dev(). My original code does NOT have such a function.
Revalidation is much more subtle and the code looks simpler in my original
version. In my original code there is a lot more recursion, symmetry and
code mirroring. Granted, while such code is shorter, and simpler, it is harder
to figure out what it does, and I feel this is exactly why we see the current
state of libsas to be so explicit, simplistic and introducing bugs. See this:
http://marc.info/?l=linux-scsi&m=131480962006471&w=2 where I described
the state of libsas recently.
> Please note, as I am *stuck* on Outlook as per company policy, the
> following inline content will likely not patch clean even emailed as
> 'Plain Text', the enclosed attached file should do the job. I have
> Cc'd
> all the folks that originated the files in libsas, as there was no
> listed MAINTAINERs.
>
> Checkpatch.pl reports clean. Patch applies cleanly to a WIDE variety of
> kernels up to latest.
>
> Sincerely -- Mark Salyzyn
>
> Cc: Luben Tuikov <tuikov@yahoo.com>
> Cc: Darrick J Wong <djwong@us.ibm.com>
> Cc: James Bottomley <jbottomley@parallels.com>
>
> Signed-off-by: Mark Salyzyn <msalyzyn@us.xyratex.com>
>
> sas_expander.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff -ru scsi-misc-2.6/drivers/scsi/libsas/sas_expander.c
> scsi-misc-2.6.new/drivers/scsi/libsas/sas_expander.c
> --- scsi-misc-2.6/drivers/scsi/libsas/sas_expander.c 2011-08-31
> 08:32:21.000000000 -0400
> +++ scsi-misc-2.6.new/drivers/scsi/libsas/sas_expander.c
> 2011-09-01 08:57:55.000000000 -0400
> @@ -1721,7 +1721,7 @@
> list_for_each_entry(ch, &ex->children, siblings) {
> if (ch->dev_type == EDGE_DEV || ch->dev_type ==
> FANOUT_DEV) {
> res = sas_find_bcast_dev(ch, src_dev);
> - if (src_dev)
> + if (*src_dev)
> return res;
> }
> }
>
--
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] 13+ messages in thread* RE: [PATCH] [SCSI]: libsas failure to revalidate domain for anything but the first expander child.
2011-09-01 17:18 ` Luben Tuikov
@ 2011-09-06 14:56 ` Mark Salyzyn
2011-09-07 17:03 ` Luben Tuikov
0 siblings, 1 reply; 13+ messages in thread
From: Mark Salyzyn @ 2011-09-06 14:56 UTC (permalink / raw)
To: Luben Tuikov, linux-scsi; +Cc: Darrick J Wong, James Bottomley
So are you suggesting a larger/different fix, or do you see this minor change as an acceptable means of resolving the specific issue of the revalidation not going beyond the first child expander? At least with this fix it will go to the first expander with a change. It was a code-bug from fumble-fingers, missing a '*', not a request to revamp ;->
I know that by spec it should not stop at the first discovered change, ignoring changes in other children, but that is a much finer corner case requiring discovery events at a higher frequency than the revalidation period can handle. Not sure if I have the stomach to wait for a rewrite of libsas ... Besides I feel libsas 'mostly works' and for today just needs a few tweaks (evolution, not revolution).
Sincerely -- Mark Salyzyn
-----Original Message-----
From: Luben Tuikov [mailto:ltuikov@yahoo.com]
Sent: Thursday, September 01, 2011 1:18 PM
To: Mark Salyzyn; linux-scsi@vger.kernel.org
Cc: Darrick J Wong; James Bottomley
Subject: Re: [PATCH] [SCSI]: libsas failure to revalidate domain for anything but the first expander child.
----- Original Message -----
> In an enclosure model where there are chaining expanders to a large body
> of storage, it was discovered that libsas, responding to a broadcast
> event change, would only revalidate the domain of first child expander
> in the list.
>
> The issue is that the pointer value to the discovered source device was
> used to break out of the loop, rather than the content of the pointer.
>
> This still remains non-compliant as the revalidate domain code is
> supposed to loop through all child expanders, and not stop at the first
> one it finds that reports a change count. However, the design of this
> routine does not allow multiple device discoveries and that would be a
> more complicated set of patches reserved for another day. We are fixing
> the glaring bug rather than refactoring the code.
Obviously I've tested this both when I was at Adaptec and at Vitesse. I'd connect
7-8 expanders, run iogen with 1000 threads to say 30-40 disks, and then unplug
the port between a level 1 and 2 expander and the I/O would quiesce, iogen would
report a subset of the disks missing, and then when the port was reestablished,
I/O would restart. However I'm not sure that Bottomley tested this scenario after
changing my code off-line before submitting it into the Linux kernel.
Now a few notes to mention: Your patch patches a function called
sas_find_bcast_dev(). My original code does NOT have such a function.
Revalidation is much more subtle and the code looks simpler in my original
version. In my original code there is a lot more recursion, symmetry and
code mirroring. Granted, while such code is shorter, and simpler, it is harder
to figure out what it does, and I feel this is exactly why we see the current
state of libsas to be so explicit, simplistic and introducing bugs. See this:
http://marc.info/?l=linux-scsi&m=131480962006471&w=2 where I described
the state of libsas recently.
> Please note, as I am *stuck* on Outlook as per company policy, the
> following inline content will likely not patch clean even emailed as
> 'Plain Text', the enclosed attached file should do the job. I have
> Cc'd
> all the folks that originated the files in libsas, as there was no
> listed MAINTAINERs.
>
> Checkpatch.pl reports clean. Patch applies cleanly to a WIDE variety of
> kernels up to latest.
>
> Sincerely -- Mark Salyzyn
>
> Cc: Luben Tuikov <tuikov@yahoo.com>
> Cc: Darrick J Wong <djwong@us.ibm.com>
> Cc: James Bottomley <jbottomley@parallels.com>
>
> Signed-off-by: Mark Salyzyn <msalyzyn@us.xyratex.com>
>
> sas_expander.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff -ru scsi-misc-2.6/drivers/scsi/libsas/sas_expander.c
> scsi-misc-2.6.new/drivers/scsi/libsas/sas_expander.c
> --- scsi-misc-2.6/drivers/scsi/libsas/sas_expander.c 2011-08-31
> 08:32:21.000000000 -0400
> +++ scsi-misc-2.6.new/drivers/scsi/libsas/sas_expander.c
> 2011-09-01 08:57:55.000000000 -0400
> @@ -1721,7 +1721,7 @@
> list_for_each_entry(ch, &ex->children, siblings) {
> if (ch->dev_type == EDGE_DEV || ch->dev_type ==
> FANOUT_DEV) {
> res = sas_find_bcast_dev(ch, src_dev);
> - if (src_dev)
> + if (*src_dev)
> return res;
> }
> }
>
______________________________________________________________________
This email may contain privileged or confidential information, which should only be used for the purpose for which it was sent by Xyratex. No further rights or licenses are granted to use such information. If you are not the intended recipient of this message, please notify the sender by return and delete it. You may not use, copy, disclose or rely on the information contained in it.
Internet email is susceptible to data corruption, interception and unauthorised amendment for which Xyratex does not accept liability. While we have taken reasonable precautions to ensure that this email is free of viruses, Xyratex does not accept liability for the presence of any computer viruses in this email, nor for any losses caused as a result of viruses.
Xyratex Technology Limited (03134912), Registered in England & Wales, Registered Office, Langstone Road, Havant, Hampshire, PO9 1SA.
The Xyratex group of companies also includes, Xyratex Ltd, registered in Bermuda, Xyratex International Inc, registered in California, Xyratex (Malaysia) Sdn Bhd registered in Malaysia, Xyratex Technology (Wuxi) Co Ltd registered in The People's Republic of China and Xyratex Japan Limited registered in Japan.
______________________________________________________________________
--
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] 13+ messages in thread* Re: [PATCH] [SCSI]: libsas failure to revalidate domain for anything but the first expander child.
2011-09-06 14:56 ` Mark Salyzyn
@ 2011-09-07 17:03 ` Luben Tuikov
0 siblings, 0 replies; 13+ messages in thread
From: Luben Tuikov @ 2011-09-07 17:03 UTC (permalink / raw)
To: Mark Salyzyn, linux-scsi; +Cc: Darrick J Wong, James Bottomley
>So are you suggesting a larger/different fix, or do you see this minor change as an acceptable means of resolving the specific issue of the revalidation not going beyond the first child expander? At least with this fix it will go to the first expander with a change. It was a code-bug from fumble-fingers, missing a '*', not a request to revamp ;->
>
Neither. I was merely stating a fact, history.
I don't mind this patch.
>I know that by spec it should not stop at the first discovered change, ignoring changes in other children, but that is a much finer corner case requiring discovery events at a higher frequency than the revalidation period can handle. Not sure if I have the stomach to wait for a rewrite of libsas ... Besides I feel libsas 'mostly works' and for today just needs a few tweaks (evolution, not revolution).
>
Yes. My original code did handle all this as event management used a clever technique to handle infinite number of events (of finite number of type of event) in finite amount of memory. Bottomley and his gang (2908d778ab3e244900c310974e1fc1c69066e450) ripped that out and added the utterly naive way or processing events as you currently see in libsas. The result is a buggy, substandard and mediocre implementation.
>Sincerely -- Mark Salyzyn
>
>-----Original Message-----
>From: Luben Tuikov [mailto:ltuikov@yahoo.com]
>Sent: Thursday, September 01, 2011 1:18 PM
>To: Mark Salyzyn; linux-scsi@vger.kernel.org
>Cc: Darrick J Wong; James Bottomley
>Subject: Re: [PATCH] [SCSI]: libsas failure to revalidate domain for anything but the first expander child.
>
>----- Original Message -----
>
>> In an enclosure model where there are chaining expanders to a large body
>> of storage, it was discovered that libsas, responding to a broadcast
>> event change, would only revalidate the domain of first child expander
>> in the list.
>>
>> The issue is that the pointer value to the discovered source device was
>> used to break out of the loop, rather than the content of the pointer.
>>
>> This still remains non-compliant as the revalidate domain code is
>> supposed to loop through all child expanders, and not stop at the first
>> one it finds that reports a change count. However, the design of this
>> routine does not allow multiple device discoveries and that would be a
>> more complicated set of patches reserved for another day. We are fixing
>> the glaring bug rather than refactoring the code.
>
>Obviously I've tested this both when I was at Adaptec and at Vitesse. I'd connect
>7-8 expanders, run iogen with 1000 threads to say 30-40 disks, and then unplug
>the port between a level 1 and 2 expander and the I/O would quiesce, iogen would
>report a subset of the disks missing, and then when the port was reestablished,
>I/O would restart. However I'm not sure that Bottomley tested this scenario after
>changing my code off-line before submitting it into the Linux kernel.
>
>Now a few notes to mention: Your patch patches a function called
>sas_find_bcast_dev(). My original code does NOT have such a function.
>Revalidation is much more subtle and the code looks simpler in my original
>version. In my original code there is a lot more recursion, symmetry and
>code mirroring. Granted, while such code is shorter, and simpler, it is harder
>to figure out what it does, and I feel this is exactly why we see the current
>state of libsas to be so explicit, simplistic and introducing bugs. See this:
>http://marc.info/?l=linux-scsi&m=131480962006471&w=2 where I described
>the state of libsas recently.
>
>> Please note, as I am *stuck* on Outlook as per company policy, the
>> following inline content will likely not patch clean even emailed as
>> 'Plain Text', the enclosed attached file should do the job. I have
>> Cc'd
>> all the folks that originated the files in libsas, as there was no
>> listed MAINTAINERs.
>>
>> Checkpatch.pl reports clean. Patch applies cleanly to a WIDE variety of
>> kernels up to latest.
>>
>> Sincerely -- Mark Salyzyn
>>
>> Cc: Luben Tuikov <tuikov@yahoo.com>
>> Cc: Darrick J Wong <djwong@us.ibm.com>
>> Cc: James Bottomley <jbottomley@parallels.com>
>>
>> Signed-off-by: Mark Salyzyn <msalyzyn@us.xyratex.com>
>>
>> sas_expander.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff -ru scsi-misc-2.6/drivers/scsi/libsas/sas_expander.c
>> scsi-misc-2.6.new/drivers/scsi/libsas/sas_expander.c
>> --- scsi-misc-2.6/drivers/scsi/libsas/sas_expander.c 2011-08-31
>> 08:32:21.000000000 -0400
>> +++ scsi-misc-2.6.new/drivers/scsi/libsas/sas_expander.c
>> 2011-09-01 08:57:55.000000000 -0400
>> @@ -1721,7 +1721,7 @@
>> list_for_each_entry(ch, &ex->children, siblings) {
>> if (ch->dev_type == EDGE_DEV || ch->dev_type ==
>> FANOUT_DEV) {
>> res = sas_find_bcast_dev(ch, src_dev);
>> - if (src_dev)
>> + if (*src_dev)
>> return res;
>> }
>> }
>>
>______________________________________________________________________
>This email may contain privileged or confidential information, which should only be used for the purpose for which it was sent by Xyratex. No further rights or licenses are granted to use such information. If you are not the intended recipient of this message, please notify the sender by return and delete it. You may not use, copy, disclose or rely on the information contained in it.
>
>Internet email is susceptible to data corruption, interception and unauthorised amendment for which Xyratex does not accept liability. While we have taken reasonable precautions to ensure that this email is free of viruses, Xyratex does not accept liability for the presence of any computer viruses in this email, nor for any losses caused as a result of viruses.
>
>Xyratex Technology Limited (03134912), Registered in England & Wales, Registered Office, Langstone Road, Havant, Hampshire, PO9 1SA.
>
>The Xyratex group of companies also includes, Xyratex Ltd, registered in Bermuda, Xyratex International Inc, registered in California, Xyratex (Malaysia) Sdn Bhd registered in Malaysia, Xyratex Technology (Wuxi) Co Ltd registered in The People's Republic of China and Xyratex Japan Limited registered in Japan.
>______________________________________________________________________
>
>
>
>
>
--
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] 13+ messages in thread
* Re: [PATCH] [SCSI]: libsas failure to revalidate domain for anything but the first expander child.
2011-09-01 13:11 ` [PATCH] [SCSI]: libsas failure to revalidate domain for anything but the first expander child Mark Salyzyn
2011-09-01 17:18 ` Luben Tuikov
@ 2011-09-07 19:26 ` Dan Williams
1 sibling, 0 replies; 13+ messages in thread
From: Dan Williams @ 2011-09-07 19:26 UTC (permalink / raw)
To: Mark Salyzyn
Cc: linux-scsi, Luben Tuikov, Darrick J Wong, James Bottomley,
Xiangliang Yu, Jack Wang
[-- Attachment #1: Type: text/plain, Size: 1855 bytes --]
On Thu, Sep 1, 2011 at 6:11 AM, Mark Salyzyn
<mark_salyzyn@us.xyratex.com> wrote:
> In an enclosure model where there are chaining expanders to a large body
> of storage, it was discovered that libsas, responding to a broadcast
> event change, would only revalidate the domain of first child expander
> in the list.
>
> The issue is that the pointer value to the discovered source device was
> used to break out of the loop, rather than the content of the pointer.
>
> This still remains non-compliant as the revalidate domain code is
> supposed to loop through all child expanders, and not stop at the first
> one it finds that reports a change count. However, the design of this
> routine does not allow multiple device discoveries and that would be a
> more complicated set of patches reserved for another day. We are fixing
> the glaring bug rather than refactoring the code.
>
> Please note, as I am *stuck* on Outlook as per company policy, the
> following inline content will likely not patch clean even emailed as
> 'Plain Text', the enclosed attached file should do the job.
Yeah, Outlook helped you out with some line wrapping. The attachment
is missing "From:", "Subject::" and the rest of the changelog, I went
ahead and attached a revised version.
> I have Cc'd
> all the folks that originated the files in libsas, as there was no
> listed MAINTAINERs.
I wouldn't mind being cc'd on all libsas patches, probably all the
other libsas lldd maintainers would like to be cc'd as well (added
Xiangliang (mvsas) and Jack (pm8001))
> Checkpatch.pl reports clean. Patch applies cleanly to a WIDE variety of
> kernels up to latest.
>
> Sincerely -- Mark Salyzyn
>
> Cc: Luben Tuikov <tuikov@yahoo.com>
> Cc: Darrick J Wong <djwong@us.ibm.com>
> Cc: James Bottomley <jbottomley@parallels.com>
Reviewed-by: Dan Williams <dan.j.williams@intel.com>
[-- Attachment #2: expander_bug.patch --]
[-- Type: text/x-patch, Size: 1867 bytes --]
From e3569cf6e71f9435af8476c6729624cf32870326 Mon Sep 17 00:00:00 2001
From: Mark Salyzyn <mark_salyzyn@us.xyratex.com>
Date: Thu, 1 Sep 2011 06:11:17 -0700
Subject: [PATCH] libsas: failure to revalidate domain for anything but the
first expander child
In an enclosure model where there are chaining expanders to a large body
of storage, it was discovered that libsas, responding to a broadcast
event change, would only revalidate the domain of first child expander
in the list.
The issue is that the pointer value to the discovered source device was
used to break out of the loop, rather than the content of the pointer.
This still remains non-compliant as the revalidate domain code is
supposed to loop through all child expanders, and not stop at the first
one it finds that reports a change count. However, the design of this
routine does not allow multiple device discoveries and that would be a
more complicated set of patches reserved for another day. We are fixing
the glaring bug rather than refactoring the code.
Cc: Luben Tuikov <tuikov@yahoo.com>
Cc: Darrick J Wong <djwong@us.ibm.com>
Cc: James Bottomley <jbottomley@parallels.com>
Reviewed-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Mark Salyzyn <msalyzyn@us.xyratex.com>
---
drivers/scsi/libsas/sas_expander.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index 874e29d..7d68517 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -1718,7 +1718,7 @@ static int sas_find_bcast_dev(struct domain_device *dev,
list_for_each_entry(ch, &ex->children, siblings) {
if (ch->dev_type == EDGE_DEV || ch->dev_type == FANOUT_DEV) {
res = sas_find_bcast_dev(ch, src_dev);
- if (src_dev)
+ if (*src_dev)
return res;
}
}
--
1.7.6
^ permalink raw reply related [flat|nested] 13+ messages in thread
end of thread, other threads:[~2011-09-07 19:26 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-07-30 0:26 [PATCH v3] libsas: export sas_alloc_task() Dan Williams
2011-08-31 13:44 ` [PATCH] [SCSI]: Allow expander T-T attachments Mark Salyzyn
2011-08-31 17:43 ` Luben Tuikov
2011-08-31 17:51 ` Mark Salyzyn
2011-09-01 15:34 ` James Bottomley
2011-09-01 15:58 ` Mark Salyzyn
2011-09-06 16:47 ` Luben Tuikov
2011-09-01 17:25 ` Luben Tuikov
2011-09-01 13:11 ` [PATCH] [SCSI]: libsas failure to revalidate domain for anything but the first expander child Mark Salyzyn
2011-09-01 17:18 ` Luben Tuikov
2011-09-06 14:56 ` Mark Salyzyn
2011-09-07 17:03 ` Luben Tuikov
2011-09-07 19:26 ` Dan Williams
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.