Re: [PATCH 06/16] BNX2I: Added code to handle the binding of an existing connection

[Mike Christie <michaelc@cs.wisc.edu>]

On 01/06/2011 12:54 AM, Or Gerlitz wrote:
> Mike Christie wrote:
>> I went a different way. In the attached patch we detect the problem when
>> binding and will force a disconnect of the old ep before binding a new one.
>> Try it out and let me know.
>
>> --- a/drivers/scsi/iscsi_tcp.c
>> +++ b/drivers/scsi/iscsi_tcp.c
>> @@ -651,8 +651,7 @@ free_addr:
>>
>>   static int
>>   iscsi_sw_tcp_conn_bind(struct iscsi_cls_session *cls_session,
>> -		       struct iscsi_cls_conn *cls_conn, uint64_t transport_eph,
>> -		       int is_leading)
>> +		       struct iscsi_cls_conn *cls_conn, uint64_t transport_eph)
>>   {
>>   	struct Scsi_Host *shost = iscsi_session_to_shost(cls_session);
>>   	struct iscsi_host *ihost = shost_priv(shost);
>> @@ -685,7 +684,7 @@ iscsi_sw_tcp_conn_bind(struct iscsi_cls_session *cls_session,
>>   	if (err)
>>   		goto free_socket;
>>
>> -	err = iscsi_conn_bind(cls_session, cls_conn, is_leading);
>> +	err = iscsi_conn_bind(cls_session, cls_conn, NULL);
>
> should this be transport_eph instead of NULL?


No. iscsi_tcp does not use the iscsi_endpoint abstraction becuase it 
actually uses normal old sockets that are created in userspace.

>
>> --- a/drivers/scsi/libiscsi.c
>> +++ b/drivers/scsi/libiscsi.c
>> @@ -3137,16 +3137,18 @@ void iscsi_conn_stop(struct iscsi_cls_conn *cls_conn, int flag)
>>   EXPORT_SYMBOL_GPL(iscsi_conn_stop);
>>
>>   int iscsi_conn_bind(struct iscsi_cls_session *cls_session,
>> -		    struct iscsi_cls_conn *cls_conn, int is_leading)
>> +		    struct iscsi_cls_conn *cls_conn,
>> +		    struct iscsi_endpoint *ep)
>>   {
>>   	struct iscsi_session *session = cls_session->dd_data;
>>   	struct iscsi_conn *conn = cls_conn->dd_data;
>>
>>   	spin_lock_bh(&session->lock);
>> -	if (is_leading)
>> -		session->leadconn = conn;
>> +	session->leadconn = conn;
>>   	spin_unlock_bh(&session->lock);
>>
>> +	ep->conn = cls_conn;
>> +	cls_conn->ep = ep;
>
> if not, it doesn't look like ep can be null here...

Will fix. Tested offload and did not test software iscsi.

>
>
>> --- a/include/scsi/libiscsi.h
>> +++ b/include/scsi/libiscsi.h
>> @@ -388,7 +388,7 @@ extern void iscsi_conn_teardown(struct iscsi_cls_conn *);
>>   extern int iscsi_conn_bind(struct iscsi_cls_session *, struct iscsi_cls_conn *,
>> -			   int);
>> +			   struct iscsi_endpoint *);
>
>> --- a/include/scsi/scsi_transport_iscsi.h
>> +++ b/include/scsi/scsi_transport_iscsi.h
>> @@ -95,7 +95,7 @@ struct iscsi_transport {
>>   				uint32_t cid);
>>   	int (*bind_conn) (struct iscsi_cls_session *session,
>>   			  struct iscsi_cls_conn *cls_conn,
>> -			  uint64_t transport_eph, int is_leading);
>> +			  uint64_t transport_eph);
>
> so we're implicitly casting from u64 to ep pointer? I guess this should be fine.
>

No. The transport->bind_conn takes the u64 id. The driver then looks 
that up and passes iscsi_bind_conn a pointer to the ep struct.