Re: [PATCH] [SCSI]: Allow expander T-T attachments

[Luben Tuikov <ltuikov@yahoo.com>]

----- 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