[PATCH] fc_transport: stop creating duplicate rport entries.

[PATCH] fc_transport: stop creating duplicate rport entries.

[Andrew Vasquez]

Current fc_transport consumers initially register rports
with an UNKNOWN role-state and follow-up with a call to
fc_remote_port_rolechg().  Modify code in
fc_remote_port_add() to scan the fc_host_rport_bindings()
array for consistent bindings regardless of role-type.
Original code would only scan bindings array for targets,
causing duplicate fc_remote_ports/rport-X:Y-Z entries to be
created for the yet-to-be-role-changed rports.

Signed-off-by: Andrew Vasquez <andrew.vasquez@qlogic.com>
---

This should go into 2.6.16.

diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/scsi_transport_fc.c
index f2c9acf..929032e 100644
--- a/drivers/scsi/scsi_transport_fc.c
+++ b/drivers/scsi/scsi_transport_fc.c
@@ -1498,8 +1498,7 @@ fc_remote_port_add(struct Scsi_Host *sho
 	}
 
 	/* Search the bindings array */
-	if (likely((ids->roles & FC_RPORT_ROLE_FCP_TARGET) &&
-		(fc_host_tgtid_bind_type(shost) != FC_TGTID_BIND_NONE))) {
+	if (fc_host_tgtid_bind_type(shost) != FC_TGTID_BIND_NONE) {
 
 		/* search for a matching consistent binding */
 

Re: [PATCH] fc_transport: stop creating duplicate rport entries.

[James Smart]

Good Catch!

-- james s

Andrew Vasquez wrote:
> Current fc_transport consumers initially register rports
> with an UNKNOWN role-state and follow-up with a call to
> fc_remote_port_rolechg().  Modify code in
> fc_remote_port_add() to scan the fc_host_rport_bindings()
> array for consistent bindings regardless of role-type.
> Original code would only scan bindings array for targets,
> causing duplicate fc_remote_ports/rport-X:Y-Z entries to be
> created for the yet-to-be-role-changed rports.
> 
> Signed-off-by: Andrew Vasquez <andrew.vasquez@qlogic.com>
> ---
> 
> This should go into 2.6.16.
> 
> diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/scsi_transport_fc.c
> index f2c9acf..929032e 100644
> --- a/drivers/scsi/scsi_transport_fc.c
> +++ b/drivers/scsi/scsi_transport_fc.c
> @@ -1498,8 +1498,7 @@ fc_remote_port_add(struct Scsi_Host *sho
>  	}
>  
>  	/* Search the bindings array */
> -	if (likely((ids->roles & FC_RPORT_ROLE_FCP_TARGET) &&
> -		(fc_host_tgtid_bind_type(shost) != FC_TGTID_BIND_NONE))) {
> +	if (fc_host_tgtid_bind_type(shost) != FC_TGTID_BIND_NONE) {
>  
>  		/* search for a matching consistent binding */
>  
> -
> 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
> 

Re: [PATCH] fc_transport: stop creating duplicate rport entries.

[Michael Reed]

James Smart wrote:
> Good Catch!
> 
> -- james s
> 
> Andrew Vasquez wrote:
>> Current fc_transport consumers initially register rports
>> with an UNKNOWN role-state and follow-up with a call to
>> fc_remote_port_rolechg(). 

So, is the above sequence necessary?  Why not just register
the rport with appropriate role and state?  This is what
LSI fusion driver (mptfc) does.  "roles" has target and
initiator set.  (Is fusion broken?)

(Not saying the fix shouldn't be there.)

Mike

<...snip...>

Re: [PATCH] fc_transport: stop creating duplicate rport entries.

[Michael Reed]

Michael Reed wrote:
> 
> James Smart wrote:
>>Good Catch!
>>
>>-- james s
>>
>>Andrew Vasquez wrote:
>>>Current fc_transport consumers initially register rports
>>>with an UNKNOWN role-state and follow-up with a call to
>>>fc_remote_port_rolechg(). 
> 
> So, is the above sequence necessary?  Why not just register
> the rport with appropriate role and state?  This is what
> LSI fusion driver (mptfc) does.  "roles" has target and
> initiator set.  (Is fusion broken?)

WHAT AM I SAYING?  roles is passed in with "target" set
unless the target is also an initiator.

Mike

> 
> (Not saying the fix shouldn't be there.)
> 
> Mike
> 
> <...snip...>
> 
> -
> 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
> 
> 

Re: [PATCH] fc_transport: stop creating duplicate rport entries.

[James Smart]

Is it necessary - e.g. register w/o role, then update it ? No, if
the lldd wants to plogi then wait for prli to occur before registering
the rport - that's fine.

However, I've found it easier, especially for non-FCP entities, to
register the rport as soon as it was detected (works for anything, non-FCP
included), then when prli completes update the role (works only w/ FCP).

mptfc is fine to register once with the roles already known.

-- james


Michael Reed wrote:
> 
> James Smart wrote:
>> Good Catch!
>>
>> -- james s
>>
>> Andrew Vasquez wrote:
>>> Current fc_transport consumers initially register rports
>>> with an UNKNOWN role-state and follow-up with a call to
>>> fc_remote_port_rolechg(). 
> 
> So, is the above sequence necessary?  Why not just register
> the rport with appropriate role and state?  This is what
> LSI fusion driver (mptfc) does.  "roles" has target and
> initiator set.  (Is fusion broken?)
> 
> (Not saying the fix shouldn't be there.)
> 
> Mike
> 
> <...snip...>
> 
> 

Re: [PATCH] fc_transport: stop creating duplicate rport entries.

[Michael Reed]

James Smart wrote:
> Is it necessary - e.g. register w/o role, then update it ? No, if
> the lldd wants to plogi then wait for prli to occur before registering
> the rport - that's fine.
> 
> However, I've found it easier, especially for non-FCP entities, to
> register the rport as soon as it was detected (works for anything, non-FCP
> included), then when prli completes update the role (works only w/ FCP).
> 
> mptfc is fine to register once with the roles already known.

I have figured out a significant difference between these two sequences

	rid->roles = FC_RPORT_ROLE_UNKNOWN | FC_RPORT_ROLE_FCP_TARGET;
	rport = fc_remote_port_add(host,channel,rid);

-and-

	rid->roles = FC_RPORT_ROLE_UNKNOWN;
	rport = fc_remote_port_add(host,channel,rid);
	rid->roles |= FC_RPORT_ROLE_FCP_TARGET;
	fc_remote_port_rolechg(rport,rid->roles);

The difference is that the former case will schedule rport->scan_work
IMMEDIATELY after the fc_remote_port_add() and the latter will wait
until the fc_remote_port_rolechg().

Why is this significant?  Because the fc_remote_port_add() clears the
dd_data.  In the first case it is possible for the scan_work to begin
prior to the driver filling in the dd_data and possibly causing a panic.
The second case allows for this sequence:

	rid->roles = FC_RPORT_ROLE_UNKNOWN;
	rport = fc_remote_port_add(host,channel,rid);

	rport->dd_data = some_important_data_item;

	rid->roles |= FC_RPORT_ROLE_FCP_TARGET;
	fc_remote_port_rolechg(rport,rid->roles);

It seems wrong, somehow, for the scan to be kicked off by the add.
Perhaps the driver should be responsible for kicking off the scan
after it knows its infrastructure is fully in place.

	fc_remote_port_initiate_target_scan(rport);

The proposed function would kick off a scan if the rport's role
include FCP_TARGET.  Otherwise, NOP.

Alternately, I suppose fc_remote_port_add() could be modified
to accept an argument pointing to the data to be placed in
dd_data (presuming dd_fcrport_size is non-zero).

	fc_remote_port_add(host,channel,rid,dd_data);

Thanks for thinking about this.

Mike

> 
> -- james
> 
> 
> Michael Reed wrote:
>>
>> James Smart wrote:
>>> Good Catch!
>>>
>>> -- james s
>>>
>>> Andrew Vasquez wrote:
>>>> Current fc_transport consumers initially register rports
>>>> with an UNKNOWN role-state and follow-up with a call to
>>>> fc_remote_port_rolechg(). 
>>
>> So, is the above sequence necessary?  Why not just register
>> the rport with appropriate role and state?  This is what
>> LSI fusion driver (mptfc) does.  "roles" has target and
>> initiator set.  (Is fusion broken?)
>>
>> (Not saying the fix shouldn't be there.)
>>
>> Mike