Patchwork [Open-FCoE,3/4] libfc: Fix timer arm debug statement

login
register
mail settings
Submitter Robert Love
Date June 10, 2013, 11:28 p.m.
Message ID <20130610232814.26115.85760.stgit@fritz>
Download mbox | patch
Permalink /patch/60/
State Superseded
Headers show

Comments

Robert Love - June 10, 2013, 11:28 p.m.
Logging in the timer arm routine doesn't provide any
context to the user who is debugging. Move the debug
logging statement to the caller such that the context
may be presented in the log statement.

Signed-off-by: Robert Love <robert.w.love@intel.com>
Tested-by: Jack Morgan<jack.morgan@intel.com>
---
 drivers/scsi/libfc/fc_exch.c |   16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)
Neil Horman - June 12, 2013, 11:50 a.m.
On Mon, Jun 10, 2013 at 04:28:14PM -0700, Robert Love wrote:
> Logging in the timer arm routine doesn't provide any
> context to the user who is debugging. Move the debug
> logging statement to the caller such that the context
> may be presented in the log statement.
> 
> Signed-off-by: Robert Love <robert.w.love@intel.com>
> Tested-by: Jack Morgan<jack.morgan@intel.com>
> ---
>  drivers/scsi/libfc/fc_exch.c |   16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/scsi/libfc/fc_exch.c b/drivers/scsi/libfc/fc_exch.c
> index 0316cbf..1707cbb 100644
> --- a/drivers/scsi/libfc/fc_exch.c
> +++ b/drivers/scsi/libfc/fc_exch.c
> @@ -360,8 +360,6 @@ static inline void fc_exch_timer_set_locked(struct fc_exch *ep,
>  	if (ep->state & (FC_EX_RST_CLEANUP | FC_EX_DONE))
>  		return;
>  
> -	FC_EXCH_DBG(ep, "Exchange timer armed : %d msecs\n", timer_msec);
> -
>  	if (queue_delayed_work(fc_exch_workqueue, &ep->timeout_work,
>  			       msecs_to_jiffies(timer_msec)))
>  		fc_exch_hold(ep);		/* hold for timer */
> @@ -612,8 +610,10 @@ static int fc_exch_abort_locked(struct fc_exch *ep,
>  		return -ENOMEM;
>  
>  	ep->esb_stat |= ESB_ST_SEQ_INIT | ESB_ST_ABNORMAL;
> -	if (timer_msec)
> +	if (timer_msec) {
> +		FC_EXCH_DBG(ep, "Exchange timer armed (exchange being aborted): %d msecs\n", timer_msec);
>  		fc_exch_timer_set_locked(ep, timer_msec);
> +	}
>  
>  	/*
>  	 * If not logged into the fabric, don't send ABTS but leave
> @@ -1288,6 +1288,7 @@ static void fc_exch_recv_abts(struct fc_exch *ep, struct fc_frame *rx_fp)
>  	if (!(ep->esb_stat & ESB_ST_REC_QUAL))
>  		fc_exch_hold(ep);		/* hold for REC_QUAL */
>  	ep->esb_stat |= ESB_ST_ABNORMAL | ESB_ST_REC_QUAL;
> +	FC_EXCH_DBG(ep, "Exchange timer armed (reveived ABTS): %d msecs\n", timer_msec);
>  	fc_exch_timer_set_locked(ep, ep->r_a_tov);
>  
>  	fp = fc_frame_alloc(ep->lp, sizeof(*ap));
> @@ -1610,8 +1611,10 @@ static void fc_exch_abts_resp(struct fc_exch *ep, struct fc_frame *fp)
>  	else
>  		fc_frame_free(fp);
>  
> -	if (has_rec)
> +	if (has_rec) {
> +		FC_EXCH_DBG(ep, "Exchange timer armed (received ABTS response, has REC): %d msecs\n", timer_msec);
>  		fc_exch_timer_set(ep, ep->r_a_tov);
> +	}
>  
>  }
>  
> @@ -2024,8 +2027,10 @@ static struct fc_seq *fc_exch_seq_send(struct fc_lport *lport,
>  	if (unlikely(lport->tt.frame_send(lport, fp)))
>  		goto err;
>  
> -	if (timer_msec)
> +	if (timer_msec) {
> +		FC_EXCH_DBG(ep, "Exchange timer armed (sending exch): %d msecs\n", timer_msec);
>  		fc_exch_timer_set_locked(ep, timer_msec);
> +	}
>  	ep->f_ctl &= ~FC_FC_FIRST_SEQ;	/* not first seq */
>  
>  	if (ep->f_ctl & FC_FC_SEQ_INIT)
> @@ -2090,6 +2095,7 @@ retry:
>  		return;
>  	}
>  	ep->esb_stat |= ESB_ST_REC_QUAL;
> +	FC_EXCH_DBG(ep, "Exchange timer armed (sending RRQ): %d msecs\n", timer_msec);
>  	fc_exch_timer_set_locked(ep, ep->r_a_tov);
>  	spin_unlock_bh(&ep->ex_lock);
>  }
> 
> _______________________________________________
> fcoe-devel mailing list
> fcoe-devel@open-fcoe.org
> http://lists.open-fcoe.org/mailman/listinfo/fcoe-devel
> 

I know its just a few call sites now, but to prevent future fan out (and people
forgetting to add a printk with the call to fc_exch_timer_set_locked), would it
be better to keep the FC_EXCH_DBG call in the function, and use
builtin_return_address and lookup_symbol_name() to resolve the context issue?

Neil

Patch

diff --git a/drivers/scsi/libfc/fc_exch.c b/drivers/scsi/libfc/fc_exch.c
index 0316cbf..1707cbb 100644
--- a/drivers/scsi/libfc/fc_exch.c
+++ b/drivers/scsi/libfc/fc_exch.c
@@ -360,8 +360,6 @@  static inline void fc_exch_timer_set_locked(struct fc_exch *ep,
 	if (ep->state & (FC_EX_RST_CLEANUP | FC_EX_DONE))
 		return;
 
-	FC_EXCH_DBG(ep, "Exchange timer armed : %d msecs\n", timer_msec);
-
 	if (queue_delayed_work(fc_exch_workqueue, &ep->timeout_work,
 			       msecs_to_jiffies(timer_msec)))
 		fc_exch_hold(ep);		/* hold for timer */
@@ -612,8 +610,10 @@  static int fc_exch_abort_locked(struct fc_exch *ep,
 		return -ENOMEM;
 
 	ep->esb_stat |= ESB_ST_SEQ_INIT | ESB_ST_ABNORMAL;
-	if (timer_msec)
+	if (timer_msec) {
+		FC_EXCH_DBG(ep, "Exchange timer armed (exchange being aborted): %d msecs\n", timer_msec);
 		fc_exch_timer_set_locked(ep, timer_msec);
+	}
 
 	/*
 	 * If not logged into the fabric, don't send ABTS but leave
@@ -1288,6 +1288,7 @@  static void fc_exch_recv_abts(struct fc_exch *ep, struct fc_frame *rx_fp)
 	if (!(ep->esb_stat & ESB_ST_REC_QUAL))
 		fc_exch_hold(ep);		/* hold for REC_QUAL */
 	ep->esb_stat |= ESB_ST_ABNORMAL | ESB_ST_REC_QUAL;
+	FC_EXCH_DBG(ep, "Exchange timer armed (reveived ABTS): %d msecs\n", timer_msec);
 	fc_exch_timer_set_locked(ep, ep->r_a_tov);
 
 	fp = fc_frame_alloc(ep->lp, sizeof(*ap));
@@ -1610,8 +1611,10 @@  static void fc_exch_abts_resp(struct fc_exch *ep, struct fc_frame *fp)
 	else
 		fc_frame_free(fp);
 
-	if (has_rec)
+	if (has_rec) {
+		FC_EXCH_DBG(ep, "Exchange timer armed (received ABTS response, has REC): %d msecs\n", timer_msec);
 		fc_exch_timer_set(ep, ep->r_a_tov);
+	}
 
 }
 
@@ -2024,8 +2027,10 @@  static struct fc_seq *fc_exch_seq_send(struct fc_lport *lport,
 	if (unlikely(lport->tt.frame_send(lport, fp)))
 		goto err;
 
-	if (timer_msec)
+	if (timer_msec) {
+		FC_EXCH_DBG(ep, "Exchange timer armed (sending exch): %d msecs\n", timer_msec);
 		fc_exch_timer_set_locked(ep, timer_msec);
+	}
 	ep->f_ctl &= ~FC_FC_FIRST_SEQ;	/* not first seq */
 
 	if (ep->f_ctl & FC_FC_SEQ_INIT)
@@ -2090,6 +2095,7 @@  retry:
 		return;
 	}
 	ep->esb_stat |= ESB_ST_REC_QUAL;
+	FC_EXCH_DBG(ep, "Exchange timer armed (sending RRQ): %d msecs\n", timer_msec);
 	fc_exch_timer_set_locked(ep, ep->r_a_tov);
 	spin_unlock_bh(&ep->ex_lock);
 }