Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 27 Sep 2018 13:35:00 -0700
From:   Nathan Chancellor <natechancellor@gmail.com>
To:     Nick Desaulniers <ndesaulniers@google.com>
Cc:     Michal.Kalderon@cavium.com, Ariel.Elior@cavium.com,
        dledford@redhat.com, jgg@ziepe.ca, linux-rdma@vger.kernel.org,
        LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] RDMA/qedr: Explicitly cast pkt->tx_dest to
 qed_ll2_tx_dest
Message-ID: <20180927203500.GA12450@flashbox>
References: <20180927011820.13608-1-natechancellor@gmail.com>
 <CAKwvOd=Oe1AAbQKcR+6K7LtZGne+-=3zREs_xVMnEbXBZJDJTQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAKwvOd=Oe1AAbQKcR+6K7LtZGne+-=3zREs_xVMnEbXBZJDJTQ@mail.gmail.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Sep 27, 2018 at 01:28:12PM -0700, Nick Desaulniers wrote:
> On Wed, Sep 26, 2018 at 6:18 PM Nathan Chancellor
> <natechancellor@gmail.com> wrote:
> >
> > Clang warns when one enumerated type is explicitly converted to another.
> >
> > drivers/infiniband/hw/qedr/qedr_roce_cm.c:198:28: warning: implicit
> > conversion from enumeration type 'enum qed_roce_ll2_tx_dest' to
> > different enumeration type 'enum qed_ll2_tx_dest' [-Wenum-conversion]
> >         ll2_tx_pkt.tx_dest = pkt->tx_dest;
> >                            ~ ~~~~~^~~~~~~
> > 1 warning generated.
> >
> > Avoid this warning by explicitly casting pkt->tx_dest to
> > qed_112_tx_dest, which has the expected values from the
> > type qed_roce_ll2_tx_dest.
> 
> But these enums are different lengths, which is problematic for this
> patch.  Is this code broken, or that it's ok for ll2_tx_pkt.tx_dest to
> have a value that's not a valid enumeration value for enum
> qed_ll2_tx_dest? (QED_LL2_TX_DEST_MAX 's value (3) is outside the
> enumeration values of enum qed_roce_ll2_tx_dest).
> 
> include/linux/qed/qed_rdma_if.h:
>  42 enum qed_roce_ll2_tx_dest {
>  43         /* Light L2 TX Destination to the Network */
>  44         QED_ROCE_LL2_TX_DEST_NW,
>  45
>  46         /* Light L2 TX Destination to the Loopback */
>  47         QED_ROCE_LL2_TX_DEST_LB,
>  48         QED_ROCE_LL2_TX_DEST_MAX
>  49 };
> 
> include/linux/qed/qed_ll2_if.h:
>  64 enum qed_ll2_tx_dest {
>  65         QED_LL2_TX_DEST_NW, /* Light L2 TX Destination to the
> Network */
>  66         QED_LL2_TX_DEST_LB, /* Light L2 TX Destination to the
> Loopback */
>  67         QED_LL2_TX_DEST_DROP, /* Light L2 Drop the TX packet */
>  68         QED_LL2_TX_DEST_MAX
>  69 };
> 
> Maybe the maintainers can clarify?
> 

My assumption was that if QED_ROCE_LL2_TX_DEST_MAX was used that the
packet would be dropped.

Turns out that QED_ROCE_LL2_TX_DEST_MAX isn't actually used anywhere in
the tree. I suppose that qed_roce_ll2_tx_dest could just be removed and
all other instances of those values could be converted to
qed_ll2_tx_dest like the rxe patch. I'll test this now and send a patch
along if it works out.

Nathan

> >
> > Reported-by: Nick Desaulniers <ndesaulniers@google.com>
> > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> > ---
> >  drivers/infiniband/hw/qedr/qedr_roce_cm.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/infiniband/hw/qedr/qedr_roce_cm.c b/drivers/infiniband/hw/qedr/qedr_roce_cm.c
> > index 85578887421b..147e0d69003d 100644
> > --- a/drivers/infiniband/hw/qedr/qedr_roce_cm.c
> > +++ b/drivers/infiniband/hw/qedr/qedr_roce_cm.c
> > @@ -195,7 +195,7 @@ static int qedr_ll2_post_tx(struct qedr_dev *dev,
> >
> >         ll2_tx_pkt.num_of_bds = 1 /* hdr */  + pkt->n_seg;
> >         ll2_tx_pkt.vlan = 0;
> > -       ll2_tx_pkt.tx_dest = pkt->tx_dest;
> > +       ll2_tx_pkt.tx_dest = (enum qed_ll2_tx_dest)pkt->tx_dest;
> >         ll2_tx_pkt.qed_roce_flavor = roce_flavor;
> >         ll2_tx_pkt.first_frag = pkt->header.baddr;
> >         ll2_tx_pkt.first_frag_len = pkt->header.len;
> > --
> > 2.19.0
> >
> 
> 
> -- 
> Thanks,
> ~Nick Desaulniers