[PATCH] ib_srpt: Make compilation with BUG=n proceed

[PATCH] ib_srpt: Make compilation with BUG=n proceed

[Bart Van Assche]

With CONFIG_BUG=n the __WARN() macro is not defined. Avoid that
building with CONFIG_BUG=n fails by replacing __WARN() with
WARN_ON(true). Also make sure that each such statement is preceeded
by an appropriate pr_err() statement.

Signed-off-by: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>
Cc: Nicholas Bellinger <nab-IzHhD5pYlfBP7FQvKIMDCQ@public.gmane.org>
Cc: Roland Dreier <roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org>
Cc: Christoph Hellwig <hch-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>
Cc: Randy Dunlap <rdunlap-/UHa2rfvQTnk1uMJSBkQmQ@public.gmane.org>
Cc: Stephen Rothwell <sfr-3FnU+UHB4dNDw9hX6IcOSA@public.gmane.org>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c |   18 +++++++++++-------
 1 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 48b3e19..f09d483 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -1464,9 +1464,9 @@ static void srpt_handle_rdma_comp(struct srpt_rdma_ch *ch,
 	} else if (opcode == SRPT_RDMA_ABORT) {
 		ioctx->rdma_aborted = true;
 	} else {
-		__WARN();
-		printk(KERN_ERR "%s[%d]: scmnd == NULL (opcode %d)", __func__,
-				__LINE__, opcode);
+		pr_err("%s[%d]: unexpected opcode %d", __func__, __LINE__,
+		       opcode);
+		WARN_ON(true);
 	}
 }
 
@@ -2737,7 +2737,9 @@ static void srpt_cm_dreq_recv(struct ib_cm_id *cm_id)
 		break;
 	case CH_DISCONNECTING:
 	case CH_DRAINING:
-		__WARN();
+		pr_err("%s[%d]: unexpected state %d\n", __func__, __LINE__,
+		       ch->state);
+		WARN_ON(true);
 		break;
 	}
 	spin_unlock_irqrestore(&ch->spinlock, flags);
@@ -2966,7 +2968,9 @@ static int srpt_write_pending(struct se_cmd *se_cmd)
 	switch (ch_state) {
 	case CH_CONNECTING:
 		/* This code should never be reached. */
-		__WARN();
+		pr_err("%s[%d]: unexpected state %d\n", __func__, __LINE__,
+		       ch_state);
+		WARN_ON(true);
 		ret = -EINVAL;
 		goto out;
 	case CH_LIVE:
@@ -3030,9 +3034,9 @@ static int srpt_queue_response(struct se_cmd *cmd)
 		ioctx->state = SRPT_STATE_MGMT_RSP_SENT;
 		break;
 	default:
-		printk(KERN_ERR "ch %p; cmd %d: unexpected command state %d\n",
+		pr_err("ch %p; cmd %d: unexpected command state %d\n",
 		       ch, ioctx->ioctx.index, ioctx->state);
-		__WARN();
+		WARN_ON(true);
 		break;
 	}
 	spin_unlock_irqrestore(&ioctx->spinlock, flags);
-- 
1.7.3.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ib_srpt: Make compilation with BUG=n proceed

[Roland Dreier]

On Thu, Nov 17, 2011 at 11:25 AM, Bart Van Assche <bvanassche@acm.org> wrote:
> +               pr_err("%s[%d]: unexpected opcode %d", __func__, __LINE__,
> +                      opcode);
> +               WARN_ON(true);

Not a big deal, but I guess this could just be

        WARN(1, "unexpected opcode %d", opcode);

etc.

 - R.

Re: [PATCH] ib_srpt: Make compilation with BUG=n proceed

[Bart Van Assche]

On Fri, Nov 18, 2011 at 12:45 AM, Roland Dreier <roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org> wrote:
> On Thu, Nov 17, 2011 at 11:25 AM, Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org> wrote:
>> +               pr_err("%s[%d]: unexpected opcode %d", __func__, __LINE__,
>> +                      opcode);
>> +               WARN_ON(true);
>
> Not a big deal, but I guess this could just be
>
>        WARN(1, "unexpected opcode %d", opcode);

As far as I can see with CONFIG_BUG=n WARN() is defined as follows:

#define WARN(condition, format...) ({                                   \
        int __ret_warn_on = !!(condition);                              \
        unlikely(__ret_warn_on);                                        \
})

So the changing pr_err(); WARN_ON(true); into WARN(1, ...) would cause
no error message to be printed if CONFIG_BUG=n. Is that the way kernel
code should behave if CONFIG_BUG=n ?

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ib_srpt: Make compilation with BUG=n proceed

[Roland Dreier]

On Thu, Nov 17, 2011 at 11:52 PM, Bart Van Assche <bvanassche@acm.org> wrote:
> As far as I can see with CONFIG_BUG=n WARN() is defined as follows:
>
> #define WARN(condition, format...) ({                                   \
>        int __ret_warn_on = !!(condition);                              \
>        unlikely(__ret_warn_on);                                        \
> })
>
> So the changing pr_err(); WARN_ON(true); into WARN(1, ...) would cause
> no error message to be printed if CONFIG_BUG=n. Is that the way kernel
> code should behave if CONFIG_BUG=n ?

I don't know the code well enough to know what it *should* do ...
pretty much all WARN (or WARN_ON or __WARN) gives over a
printk is that it includes the backtrace.  Which might not be particularly
useful in this case.

So maybe pr_err() without WARN_ON is the best thing.  I was
just responding microlocally to the couple of lines here without
thinking through what output is actually useful.

What are these error messages for?  A "can never happen" bug
in the ib_srpt logic, or something that could be triggered by an
insane initiator sending a bogus sequence?

 - R.

Re: [PATCH] ib_srpt: Make compilation with BUG=n proceed

[Bart Van Assche]

On Fri, Nov 18, 2011 at 9:10 PM, Roland Dreier <roland@purestorage.com> wrote:
> On Thu, Nov 17, 2011 at 11:52 PM, Bart Van Assche <bvanassche@acm.org> wrote:
> > As far as I can see with CONFIG_BUG=n WARN() is defined as follows:
> >
> > #define WARN(condition, format...) ({                                   \
> >        int __ret_warn_on = !!(condition);                              \
> >        unlikely(__ret_warn_on);                                        \
> > })
> >
> > So the changing pr_err(); WARN_ON(true); into WARN(1, ...) would cause
> > no error message to be printed if CONFIG_BUG=n. Is that the way kernel
> > code should behave if CONFIG_BUG=n ?
>
> I don't know the code well enough to know what it *should* do ...
> pretty much all WARN (or WARN_ON or __WARN) gives over a
> printk is that it includes the backtrace.  Which might not be particularly
> useful in this case.
>
> So maybe pr_err() without WARN_ON is the best thing.  I was
> just responding microlocally to the couple of lines here without
> thinking through what output is actually useful.

According to the description in init/Kconfig, BUG=n should only be
used on embedded systems with no facilities for error reporting. I do
not expect that anyone will miss the pr_err() output on such a system.

> What are these error messages for?  A "can never happen" bug
> in the ib_srpt logic, or something that could be triggered by an
> insane initiator sending a bogus sequence?

These messages are present in code paths that should never be reached,
no matter how the initiator or the IB network behaves. These messages
have been useful while making ib_srpt robust against IB cable pulling
- see e.g. http://www.mail-archive.com/linux-rdma@vger.kernel.org/msg09664.html.

Bart.

[PATCH] ib_srpt: Make compilation with BUG=n proceed

[Bart Van Assche]

With CONFIG_BUG=n the __WARN() macro is not defined. Avoid that
building with CONFIG_BUG=n fails by changing pr_err(...);
_WARN() into WARN(true, ...).

Signed-off-by: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>
Cc: Nicholas Bellinger <nab-IzHhD5pYlfBP7FQvKIMDCQ@public.gmane.org>
Cc: Roland Dreier <roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org>
Cc: Christoph Hellwig <hch-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>
Cc: Randy Dunlap <rdunlap-/UHa2rfvQTnk1uMJSBkQmQ@public.gmane.org>
Cc: Stephen Rothwell <sfr-3FnU+UHB4dNDw9hX6IcOSA@public.gmane.org>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c |   14 +++++---------
 1 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 49ae767..6376e2a 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -1462,9 +1462,7 @@ static void srpt_handle_rdma_comp(struct srpt_rdma_ch *ch,
 	} else if (opcode == SRPT_RDMA_ABORT) {
 		ioctx->rdma_aborted = true;
 	} else {
-		__WARN();
-		printk(KERN_ERR "%s[%d]: scmnd == NULL (opcode %d)", __func__,
-				__LINE__, opcode);
+		WARN(true, "unexpected opcode %d\n", opcode);
 	}
 }
 
@@ -2735,7 +2733,7 @@ static void srpt_cm_dreq_recv(struct ib_cm_id *cm_id)
 		break;
 	case CH_DISCONNECTING:
 	case CH_DRAINING:
-		__WARN();
+		WARN(true, "unexpected channel state %d\n", ch->state);
 		break;
 	}
 	spin_unlock_irqrestore(&ch->spinlock, flags);
@@ -2963,8 +2961,7 @@ static int srpt_write_pending(struct se_cmd *se_cmd)
 	ch_state = ch->state;
 	switch (ch_state) {
 	case CH_CONNECTING:
-		/* This code should never be reached. */
-		__WARN();
+		WARN(true, "unexpected channel state %d\n", ch_state);
 		ret = -EINVAL;
 		goto out;
 	case CH_LIVE:
@@ -3028,9 +3025,8 @@ static int srpt_queue_response(struct se_cmd *cmd)
 		ioctx->state = SRPT_STATE_MGMT_RSP_SENT;
 		break;
 	default:
-		printk(KERN_ERR "ch %p; cmd %d: unexpected command state %d\n",
-		       ch, ioctx->ioctx.index, ioctx->state);
-		__WARN();
+		WARN(true, "ch %p; cmd %d: unexpected command state %d\n",
+		     ch, ioctx->ioctx.index, ioctx->state);
 		break;
 	}
 	spin_unlock_irqrestore(&ioctx->spinlock, flags);
-- 
1.7.3.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html