On Mon, Jun 04, 2018 at 07:11:47PM +0300, Leon Romanovsky wrote:
> On Mon, Jun 04, 2018 at 09:51:20AM -0600, Jason Gunthorpe wrote:
> > On Sat, Jun 02, 2018 at 03:43:02PM +0300, Leon Romanovsky wrote:
> > > On Thu, May 31, 2018 at 09:02:50PM -0400, Doug Ledford wrote:
> > > > On Tue, 2018-05-29 at 14:56 +0300, Leon Romanovsky wrote:
> > > > > From: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> > > > >
> > > > > Let's perform checks in-place instead of BUG_ONs.
> > > > >
> > > > > Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> > > > > drivers/infiniband/core/mad.c | 11 +++++++----
> > > > > 1 file changed, 7 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/drivers/infiniband/core/mad.c b/drivers/infiniband/core/mad.c
> > > > > index 6a205224582d..c37079f86341 100644
> > > > > +++ b/drivers/infiniband/core/mad.c
> > > > > @@ -1556,7 +1556,8 @@ static int add_oui_reg_req(struct ib_mad_reg_req *mad_reg_req,
> > > > > mad_reg_req->oui, 3)) {
> > > > > method = &(*vendor_table)->vendor_class[
> > > > > vclass]->method_table[i];
> > > > > - BUG_ON(!*method);
> > > > > + if (!*method)
> > > > > + goto error3;
> > > > > goto check_in_use;
> > > > > }
> > > > > }
> > > > > @@ -1566,10 +1567,12 @@ static int add_oui_reg_req(struct ib_mad_reg_req *mad_reg_req,
> > > > > vclass]->oui[i])) {
> > > > > method = &(*vendor_table)->vendor_class[
> > > > > vclass]->method_table[i];
> > > > > - BUG_ON(*method);
> > > > > /* Allocate method table for this OUI */
> > > > > - if ((ret = allocate_method_table(method)))
> > > > > - goto error3;
> > > > > + if (*method) {
> > > > ^
> > > > Shouldn't this be (!*method)?
> > >
> > > Doug,
> > >
> > > I thought over night what I wanted in this patch and what went wrong.
> > >
> > > My intention was:
> > >
> > > if (*method)
> > > goto error3;
> > >
> > > ret = allocate_method_table(method);
> > > if (ret)
> > > oto error3;
> > >
> > > I see that you didn't update your for-next yet, can you fix the patch
> > > prior updating for-next?
> >
> > Too late now.. but I think it should read like this:
> >
> > @@ -1556,7 +1556,9 @@ static int add_oui_reg_req(struct ib_mad_reg_req *mad_reg_req,
> > mad_reg_req->oui, 3)) {
> > method = &(*vendor_table)->vendor_class[
> > vclass]->method_table[i];
> > - BUG_ON(!*method);
> > + /* method and oui are out of sync. */
> > + if (WARN(!*method))
>
> IMHO, it goes into another extreme - addition of WARN* statements in too
> many places. I didn't want to add them into the impossible flows.
WARN should be used to mark impossible flows, that is a right use of
WARN. If the original author thought the code was tricky enough to
warrant a check then it should remain as a WARN.
The better way to handle this is to not create impossible flows like
this in the first place, like why is the oui table and method table
seperate like this?
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
