Re: [RFC][PATCH 2/6] fnic: add fnic_scsi.c and fnic_io.h.

James Smart
2008-08-25T19:15:33Z

Mike Christie wrote:
> jeykholt@cisco.com wrote:
>> fnic: add fnic_scsi.c and fnic_io.h.
>>
>> fnic_scsi.c contains the FCP SCSI handling as well as firmware reset
>> and FLOGI registration handling.
>>
>
> I just looked at this one function, because I was fixing the same code
> in fcoe.ko/libfc:fc_fcp.c
>
>
>> +int fnic_reset(struct Scsi_Host *shost)
>> +{
>> + struct fc_lport *lp;
>> + struct fnic *fnic;
>> + unsigned long flags;
>> + int ret = SUCCESS;
>> + enum fnic_state old_state;
>> + DECLARE_COMPLETION_ONSTACK(reset_wait);
>> +
>> + lp = shost_priv(shost);
>> + fnic = lp->drv_priv;
>> +
>> + printk(KERN_DEBUG DFX "fnic_reset called\n", fnic->fnic_no);
>> +
>> + /* Issue firmware reset */
>> + spin_lock_irqsave(&fnic->fnic_lock, flags);
>> + fnic->reset_wait = &reset_wait;
>> + old_state = fnic->state;
>> + fnic->state = FNIC_IN_FC_TRANS_ETH_MODE;
>> + vnic_dev_del_addr(fnic->vdev, fnic->data_src_addr);
>> + spin_unlock_irqrestore(&fnic->fnic_lock, flags);
>> +
>> + if (fnic_fw_reset_handler(fnic)) {
>> + spin_lock_irqsave(&fnic->fnic_lock, flags);
>> + ret = FAILED;
>> + if (fnic->state == FNIC_IN_FC_TRANS_ETH_MODE)
>> + fnic->state = old_state;
>> + fnic->reset_wait = NULL;
>> + spin_unlock_irqrestore(&fnic->fnic_lock, flags);
>> + goto fnic_reset_end;
>> + }
>> +
>> + /* fw reset is issued, now wait for it to complete */
>> + wait_for_completion_timeout(&reset_wait,
>> + msecs_to_jiffies(FNIC_HOST_RESET_TIMEOUT));
>> +
>> + /* Check for status */
>> + spin_lock_irqsave(&fnic->fnic_lock, flags);
>> + fnic->reset_wait = NULL;
>> + ret = (fnic->state == FNIC_IN_ETH_MODE) ? SUCCESS : FAILED;
>> + spin_unlock_irqrestore(&fnic->fnic_lock, flags);
>> +
>> + /* Now reset local port, this will clean up libFC exchanges,
>> + * reset remote port sessions, and if link is up, begin flogi
>> + */
>> + fc_lport_lock(lp);
>> + if (lp->tt.lport_reset(lp))
>> + ret = FAILED;
>
>
> The problem here is that this only starts the login. When fnic_reset
> returns for the scsi eh path, scsi-ml is going to send a TUR to make
> sure that we are ready to go. If we are not the devices will be
> offlined. So unless it is a really quick relogin we are going to offline
> the devices by accident.

Well - what should be happening is - prior to the reset or as part of
it, the fc transport fc_remote_port_delete() call should be made on all
those remote ports that connectivity is about to be terminated on. This
will place all the associated targets/luns on those rports into a
blocked state, and start the devloss timer on them. This will suspend
the eh path as well. Thus, things suspend until either the driver/fcoe
stack re-login's and re-calls the transport with the same remote port
(thus the transport will unblock the targets/luns), or devloss_tmo
expires, at which time it is correct to report loss of connectivity.

Of course, all this assumes the fc_host stays in existence.

>
> For fc_fcp.c I added a hokey loop and wait like some other drivers. We
> could instead have libfc notify any waiters of a state change here. We
> could also do a rport blocked timedout helper, convert the fc drivers
> and use it here so we only wait for the login to complete or for the
> port block to fail.

-- james s