Re: [PATCH] RDMA/qedr: Explicitly cast pkt->tx_dest to qed_ll2_tx_dest

[Nathan Chancellor <natechancellor@gmail.com>]

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