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

[Luben Tuikov <ltuikov@yahoo.com>]

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