[Open-FCoE] [RFC PATCH 02/28] libfc:Add a callback to the fc_lport for fc_rport events

Robert Love robert.w.love at intel.com
Fri Sep 12 16:12:38 UTC 2008


On Thu, 2008-09-11 at 23:07 -0700, Joe Eykholt wrote:
> Robert Love wrote:
> > We want to know when an rport has successfully logged in
> > or not. The lport needs to know if the NS's rport is READY
> > or not. If it's not READY then the lport needs to decide
> > what it wants to do. The rport and NS shouldn't be
> > making decisions for the lport.
> > 
> > This patch creates a callback function for the lport and calls
> > it when the rport is READY.
> > 
> > The lport will call dns_register when it sees that the NS rport
> > is READY.
> > 
> > Signed-off-by: Robert Love <robert.w.love at intel.com>
> > ---
> > 
> >  drivers/scsi/libfc/fc_lport.c |   22 ++++++++++++++++++++++
> >  drivers/scsi/libfc/fc_rport.c |    8 +-------
> >  include/scsi/libfc/libfc.h    |    8 ++++++++
> >  3 files changed, 31 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/scsi/libfc/fc_lport.c b/drivers/scsi/libfc/fc_lport.c
> > index b390a32..7dc09f7 100644
> > --- a/drivers/scsi/libfc/fc_lport.c
> > +++ b/drivers/scsi/libfc/fc_lport.c
> > @@ -58,6 +58,25 @@ static int fc_frame_drop(struct fc_lport *lp, struct fc_frame *fp)
> >  	return 0;
> >  }
> >  
> > +static int fc_lport_rport_event(struct fc_lport *lport, struct fc_rport *rport,
> > +				enum fc_lport_event event)
> > +{
> > +	fc_lport_lock(lport);
> > +	if (rport->port_id == FC_FID_DIR_SERV) {
> > +		switch (event) {
> > +		case LPORT_EV_RPORT_CREATED:
> > +			lport->tt.dns_register(lport);
> > +			break;
> > +		case LPORT_EV_RPORT_FAILED:
> > +			fc_lport_enter_reset(lport);
> > +			break;
> > +		}
> > +	}
> > +	fc_lport_unlock(lport);
> > +
> > +	return 0;
> > +}
> 
> This could do the rport_id check before grabbing the lport lock.
> 
> > +
> >  static const char *fc_lport_state(struct fc_lport *lp)
> >  {
> >  	const char *cp;
> > @@ -921,6 +940,9 @@ int fc_lport_init(struct fc_lport *lp)
> >  	if (!lp->tt.lport_logout)
> >  		lp->tt.lport_logout = fc_lport_logout;
> >  
> > +	if (!lp->tt.lport_event)
> > +		lp->tt.lport_event = fc_lport_rport_event;
> > +
> >  	return 0;
> >  }
> >  EXPORT_SYMBOL(fc_lport_init);
> > diff --git a/drivers/scsi/libfc/fc_rport.c b/drivers/scsi/libfc/fc_rport.c
> > index c7360db..012200b 100644
> > --- a/drivers/scsi/libfc/fc_rport.c
> > +++ b/drivers/scsi/libfc/fc_rport.c
> > @@ -340,13 +340,7 @@ static void fc_rport_enter_ready(struct fc_rport *rport)
> >  	if (fc_rp_debug)
> >  		FC_DBG("remote %6x ready\n", rport->port_id);
> >  
> > -	if (rport == lport->dns_rp &&
> > -	    lport->state == LPORT_ST_DNS) {
> > -		fc_lport_lock(lport);
> > -		del_timer(&lport->state_timer);
> > -		lport->tt.dns_register(lport);
> > -		fc_lport_unlock(lport);
> > -	}
> > +	lport->tt.lport_event(lport, rport, LPORT_EV_RPORT_CREATED);
> >  }
> 
> I think it's safe for the rport code to do the callback without
> holding the rport lock (check that), and that would help fix the lock
> dependency problems.  Although that means doing the callback
> outside of the rport_enter_* functions, and might be awkward.

Yeah, my problem with the solution below is that without the rport lock
held the following can happen.

	event = rdata->event;
	rdata->event = LPORT_EV_RPORT_NONE;
	spin_unlock_bh(&rdata->rp_lock);
	*** RESET ***
	if (event != LPORT_EV_RPORT_NONE)
		lport->tt.lport_event(lport, rport, event);

That would create a race as to whether the rport object would be freed
before or after the lport event handler checks the FID. I guess I don't
need to pass an rport pointer, I can just pass the FID. 


> One way to achieve this is to have an indication in the rport that
> tells fc_rport_unlock() to do the callback after clearing the
> flag and dropping the lock.
> 
> In case this isn't obvious, what I'm talking about is:  add a
> field in fc_rport_libfc_priv called event.  Instead of calling the
> event handler above, it would merely do (I didn't check all the
> names, but you'll get the idea):
> 
> 	rdata->event = LPORT_EV_RPORT_CREATED;
> 
> Then in unlock:
> 
> +	event = rdata->event;
> +	rdata->event = LPORT_EV_RPORT_NONE;
> 	spin_unlock_bh(&rdata->rp_lock);
> +	if (event != LPORT_EV_RPORT_NONE)
> +		lport->tt.lport_event(lport, rport, event);
> 
> >  
> >  static void fc_rport_enter_start(struct fc_rport *rport)
> > diff --git a/include/scsi/libfc/libfc.h b/include/scsi/libfc/libfc.h
> > index b139aed..782c862 100644
> > --- a/include/scsi/libfc/libfc.h
> > +++ b/include/scsi/libfc/libfc.h
> > @@ -100,6 +100,11 @@ enum fc_lport_state {
> >  	LPORT_ST_RESET
> >  };
> >  
> > +enum fc_lport_event {
> > +	LPORT_EV_RPORT_CREATED = 0,
> > +	LPORT_EV_RPORT_FAILED
> > +};
> > +
> >  enum fc_rport_state {
> >  	RPORT_ST_NONE = 0,
> >  	RPORT_ST_INIT,		/* initialized */
> > @@ -320,6 +325,9 @@ struct libfc_function_template {
> >  	int (*lport_reset)(struct fc_lport *);
> >  	int (*lport_logout)(struct fc_lport *);
> >  
> > +	int (*lport_event)(struct fc_lport *, struct fc_rport *,
> > +			   enum fc_lport_event);
> > +
> >  	/**
> >  	 * Remote Port interfaces
> >  	 */
> > 
> > _______________________________________________
> > devel mailing list
> > devel at open-fcoe.org
> > http://www.open-fcoe.org/mailman/listinfo/devel
> 
> _______________________________________________
> devel mailing list
> devel at open-fcoe.org
> http://www.open-fcoe.org/mailman/listinfo/devel




More information about the devel mailing list