On Mon, Jun 04, 2018 at 10:15:37AM -0600, Jason Gunthorpe wrote:
> 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?
I saw it, but decided do not explore it too much, at least it works.
>
> 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
Attachment:
signature.asc
Description: PGP signature
