linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] RDMA/qedr: Explicitly cast pkt->tx_dest to qed_ll2_tx_dest
@ 2018-09-27  1:18 Nathan Chancellor
  2018-09-27 20:28 ` Nick Desaulniers
  2018-09-27 20:55 ` [PATCH v2] RDMA/qedr: Remove enumerated type qed_roce_ll2_tx_dest Nathan Chancellor
  0 siblings, 2 replies; 8+ messages in thread
From: Nathan Chancellor @ 2018-09-27  1:18 UTC (permalink / raw)
  To: Michal Kalderon, Ariel Elior, Doug Ledford, Jason Gunthorpe
  Cc: linux-rdma, linux-kernel, Nick Desaulniers, Nathan Chancellor

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.

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


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] RDMA/qedr: Explicitly cast pkt->tx_dest to qed_ll2_tx_dest
  2018-09-27  1:18 [PATCH] RDMA/qedr: Explicitly cast pkt->tx_dest to qed_ll2_tx_dest Nathan Chancellor
@ 2018-09-27 20:28 ` Nick Desaulniers
  2018-09-27 20:35   ` Nathan Chancellor
  2018-09-27 20:55 ` [PATCH v2] RDMA/qedr: Remove enumerated type qed_roce_ll2_tx_dest Nathan Chancellor
  1 sibling, 1 reply; 8+ messages in thread
From: Nick Desaulniers @ 2018-09-27 20:28 UTC (permalink / raw)
  To: Nathan Chancellor
  Cc: Michal.Kalderon, Ariel.Elior, dledford, jgg, linux-rdma, LKML

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?

>
> 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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] RDMA/qedr: Explicitly cast pkt->tx_dest to qed_ll2_tx_dest
  2018-09-27 20:28 ` Nick Desaulniers
@ 2018-09-27 20:35   ` Nathan Chancellor
  2018-09-27 20:41     ` Nick Desaulniers
  0 siblings, 1 reply; 8+ messages in thread
From: Nathan Chancellor @ 2018-09-27 20:35 UTC (permalink / raw)
  To: Nick Desaulniers
  Cc: Michal.Kalderon, Ariel.Elior, dledford, jgg, linux-rdma, LKML

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] RDMA/qedr: Explicitly cast pkt->tx_dest to qed_ll2_tx_dest
  2018-09-27 20:35   ` Nathan Chancellor
@ 2018-09-27 20:41     ` Nick Desaulniers
  0 siblings, 0 replies; 8+ messages in thread
From: Nick Desaulniers @ 2018-09-27 20:41 UTC (permalink / raw)
  To: Nathan Chancellor
  Cc: Michal.Kalderon, Ariel.Elior, dledford, jgg, linux-rdma, LKML

On Thu, Sep 27, 2018 at 1:35 PM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> 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.

Even better.  Sometimes removing code is the correct solution.  Number
of lines of code and number of bugs are positively correlated, so the
less code sometimes the better.

>
> 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



-- 
Thanks,
~Nick Desaulniers

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2] RDMA/qedr: Remove enumerated type qed_roce_ll2_tx_dest
  2018-09-27  1:18 [PATCH] RDMA/qedr: Explicitly cast pkt->tx_dest to qed_ll2_tx_dest Nathan Chancellor
  2018-09-27 20:28 ` Nick Desaulniers
@ 2018-09-27 20:55 ` Nathan Chancellor
  2018-09-27 22:29   ` Nick Desaulniers
  2018-09-28 16:14   ` Jason Gunthorpe
  1 sibling, 2 replies; 8+ messages in thread
From: Nathan Chancellor @ 2018-09-27 20:55 UTC (permalink / raw)
  To: Michal Kalderon, Ariel Elior, Doug Ledford, Jason Gunthorpe
  Cc: linux-rdma, linux-kernel, Nick Desaulniers, Nathan Chancellor

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.

Turns out that QED_ROCE_LL2_TX_DEST_NW and QED_ROCE_LL2_TX_DEST_LB are
only used once in the whole tree and QED_ROCE_LL2_TX_DEST_MAX is used
nowhere. Remove them and use the equivalent values from qed_ll2_tx_dest
in their place.

Reported-by: Nick Desaulniers <ndesaulniers@google.com>
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---

v1 -> v2:

* Rather than using an explicit cast, just convert the uses to the
  appropriate values and delete the duplicated enum.

 drivers/infiniband/hw/qedr/qedr_roce_cm.c |  4 ++--
 include/linux/qed/qed_rdma_if.h           | 11 +----------
 2 files changed, 3 insertions(+), 12 deletions(-)

diff --git a/drivers/infiniband/hw/qedr/qedr_roce_cm.c b/drivers/infiniband/hw/qedr/qedr_roce_cm.c
index 85578887421b..e1ac2fd60bb1 100644
--- a/drivers/infiniband/hw/qedr/qedr_roce_cm.c
+++ b/drivers/infiniband/hw/qedr/qedr_roce_cm.c
@@ -519,9 +519,9 @@ static inline int qedr_gsi_build_packet(struct qedr_dev *dev,
 	}
 
 	if (ether_addr_equal(udh.eth.smac_h, udh.eth.dmac_h))
-		packet->tx_dest = QED_ROCE_LL2_TX_DEST_LB;
+		packet->tx_dest = QED_LL2_TX_DEST_LB;
 	else
-		packet->tx_dest = QED_ROCE_LL2_TX_DEST_NW;
+		packet->tx_dest = QED_LL2_TX_DEST_NW;
 
 	packet->roce_mode = roce_mode;
 	memcpy(packet->header.vaddr, ud_header_buffer, header_size);
diff --git a/include/linux/qed/qed_rdma_if.h b/include/linux/qed/qed_rdma_if.h
index df4d13f7e191..d15f8e4815e3 100644
--- a/include/linux/qed/qed_rdma_if.h
+++ b/include/linux/qed/qed_rdma_if.h
@@ -39,15 +39,6 @@
 #include <linux/qed/qed_ll2_if.h>
 #include <linux/qed/rdma_common.h>
 
-enum qed_roce_ll2_tx_dest {
-	/* Light L2 TX Destination to the Network */
-	QED_ROCE_LL2_TX_DEST_NW,
-
-	/* Light L2 TX Destination to the Loopback */
-	QED_ROCE_LL2_TX_DEST_LB,
-	QED_ROCE_LL2_TX_DEST_MAX
-};
-
 #define QED_RDMA_MAX_CNQ_SIZE               (0xFFFF)
 
 /* rdma interface */
@@ -581,7 +572,7 @@ struct qed_roce_ll2_packet {
 	int n_seg;
 	struct qed_roce_ll2_buffer payload[RDMA_MAX_SGE_PER_SQ_WQE];
 	int roce_mode;
-	enum qed_roce_ll2_tx_dest tx_dest;
+	enum qed_ll2_tx_dest tx_dest;
 };
 
 enum qed_rdma_type {
-- 
2.19.0


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] RDMA/qedr: Remove enumerated type qed_roce_ll2_tx_dest
  2018-09-27 20:55 ` [PATCH v2] RDMA/qedr: Remove enumerated type qed_roce_ll2_tx_dest Nathan Chancellor
@ 2018-09-27 22:29   ` Nick Desaulniers
  2018-09-28  8:06     ` Kalderon, Michal
  2018-09-28 16:14   ` Jason Gunthorpe
  1 sibling, 1 reply; 8+ messages in thread
From: Nick Desaulniers @ 2018-09-27 22:29 UTC (permalink / raw)
  To: Nathan Chancellor
  Cc: Michal.Kalderon, Ariel.Elior, dledford, jgg, linux-rdma, LKML

On Thu, Sep 27, 2018 at 1:57 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.
>
> Turns out that QED_ROCE_LL2_TX_DEST_NW and QED_ROCE_LL2_TX_DEST_LB are
> only used once in the whole tree and QED_ROCE_LL2_TX_DEST_MAX is used
> nowhere. Remove them and use the equivalent values from qed_ll2_tx_dest
> in their place.
>
> Reported-by: Nick Desaulniers <ndesaulniers@google.com>
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
>
> v1 -> v2:
>
> * Rather than using an explicit cast, just convert the uses to the
>   appropriate values and delete the duplicated enum.
>
>  drivers/infiniband/hw/qedr/qedr_roce_cm.c |  4 ++--
>  include/linux/qed/qed_rdma_if.h           | 11 +----------
>  2 files changed, 3 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/infiniband/hw/qedr/qedr_roce_cm.c b/drivers/infiniband/hw/qedr/qedr_roce_cm.c
> index 85578887421b..e1ac2fd60bb1 100644
> --- a/drivers/infiniband/hw/qedr/qedr_roce_cm.c
> +++ b/drivers/infiniband/hw/qedr/qedr_roce_cm.c
> @@ -519,9 +519,9 @@ static inline int qedr_gsi_build_packet(struct qedr_dev *dev,
>         }
>
>         if (ether_addr_equal(udh.eth.smac_h, udh.eth.dmac_h))
> -               packet->tx_dest = QED_ROCE_LL2_TX_DEST_LB;
> +               packet->tx_dest = QED_LL2_TX_DEST_LB;
>         else
> -               packet->tx_dest = QED_ROCE_LL2_TX_DEST_NW;
> +               packet->tx_dest = QED_LL2_TX_DEST_NW;

Yep, looks like these were the only two sites using enum
qed_roce_ll2_tx_dest in the whole kernel.  The new enum values match
the previous enum values, so this is no functional change while
simplifying the code (one less enum to get wrong).
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

>
>         packet->roce_mode = roce_mode;
>         memcpy(packet->header.vaddr, ud_header_buffer, header_size);
> diff --git a/include/linux/qed/qed_rdma_if.h b/include/linux/qed/qed_rdma_if.h
> index df4d13f7e191..d15f8e4815e3 100644
> --- a/include/linux/qed/qed_rdma_if.h
> +++ b/include/linux/qed/qed_rdma_if.h
> @@ -39,15 +39,6 @@
>  #include <linux/qed/qed_ll2_if.h>
>  #include <linux/qed/rdma_common.h>
>
> -enum qed_roce_ll2_tx_dest {
> -       /* Light L2 TX Destination to the Network */
> -       QED_ROCE_LL2_TX_DEST_NW,
> -
> -       /* Light L2 TX Destination to the Loopback */
> -       QED_ROCE_LL2_TX_DEST_LB,
> -       QED_ROCE_LL2_TX_DEST_MAX
> -};
> -
>  #define QED_RDMA_MAX_CNQ_SIZE               (0xFFFF)
>
>  /* rdma interface */
> @@ -581,7 +572,7 @@ struct qed_roce_ll2_packet {
>         int n_seg;
>         struct qed_roce_ll2_buffer payload[RDMA_MAX_SGE_PER_SQ_WQE];
>         int roce_mode;
> -       enum qed_roce_ll2_tx_dest tx_dest;
> +       enum qed_ll2_tx_dest tx_dest;
>  };
>
>  enum qed_rdma_type {
> --
> 2.19.0
>


-- 
Thanks,
~Nick Desaulniers

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH v2] RDMA/qedr: Remove enumerated type qed_roce_ll2_tx_dest
  2018-09-27 22:29   ` Nick Desaulniers
@ 2018-09-28  8:06     ` Kalderon, Michal
  0 siblings, 0 replies; 8+ messages in thread
From: Kalderon, Michal @ 2018-09-28  8:06 UTC (permalink / raw)
  To: Nick Desaulniers, Nathan Chancellor
  Cc: Elior, Ariel, dledford, jgg, linux-rdma, LKML

> From: Nick Desaulniers <ndesaulniers@google.com>
> Sent: Friday, September 28, 2018 1:30 AM
> 
> External Email
> 
> On Thu, Sep 27, 2018 at 1:57 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.
> >
> > Turns out that QED_ROCE_LL2_TX_DEST_NW and
> QED_ROCE_LL2_TX_DEST_LB are
> > only used once in the whole tree and QED_ROCE_LL2_TX_DEST_MAX is
> used
> > nowhere. Remove them and use the equivalent values from
> > qed_ll2_tx_dest in their place.
> >
> > Reported-by: Nick Desaulniers <ndesaulniers@google.com>
> > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> > ---
> >
> > v1 -> v2:
> >
> > * Rather than using an explicit cast, just convert the uses to the
> >   appropriate values and delete the duplicated enum.
> >
> >  drivers/infiniband/hw/qedr/qedr_roce_cm.c |  4 ++--
> >  include/linux/qed/qed_rdma_if.h           | 11 +----------
> >  2 files changed, 3 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/infiniband/hw/qedr/qedr_roce_cm.c
> > b/drivers/infiniband/hw/qedr/qedr_roce_cm.c
> > index 85578887421b..e1ac2fd60bb1 100644
> > --- a/drivers/infiniband/hw/qedr/qedr_roce_cm.c
> > +++ b/drivers/infiniband/hw/qedr/qedr_roce_cm.c
> > @@ -519,9 +519,9 @@ static inline int qedr_gsi_build_packet(struct
> qedr_dev *dev,
> >         }
> >
> >         if (ether_addr_equal(udh.eth.smac_h, udh.eth.dmac_h))
> > -               packet->tx_dest = QED_ROCE_LL2_TX_DEST_LB;
> > +               packet->tx_dest = QED_LL2_TX_DEST_LB;
> >         else
> > -               packet->tx_dest = QED_ROCE_LL2_TX_DEST_NW;
> > +               packet->tx_dest = QED_LL2_TX_DEST_NW;
> 
> Yep, looks like these were the only two sites using enum
> qed_roce_ll2_tx_dest in the whole kernel.  The new enum values match the
> previous enum values, so this is no functional change while simplifying the
> code (one less enum to get wrong).
> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> 
> >
> >         packet->roce_mode = roce_mode;
> >         memcpy(packet->header.vaddr, ud_header_buffer, header_size);
> > diff --git a/include/linux/qed/qed_rdma_if.h
> > b/include/linux/qed/qed_rdma_if.h index df4d13f7e191..d15f8e4815e3
> > 100644
> > --- a/include/linux/qed/qed_rdma_if.h
> > +++ b/include/linux/qed/qed_rdma_if.h
> > @@ -39,15 +39,6 @@
> >  #include <linux/qed/qed_ll2_if.h>
> >  #include <linux/qed/rdma_common.h>
> >
> > -enum qed_roce_ll2_tx_dest {
> > -       /* Light L2 TX Destination to the Network */
> > -       QED_ROCE_LL2_TX_DEST_NW,
> > -
> > -       /* Light L2 TX Destination to the Loopback */
> > -       QED_ROCE_LL2_TX_DEST_LB,
> > -       QED_ROCE_LL2_TX_DEST_MAX
> > -};
> > -
> >  #define QED_RDMA_MAX_CNQ_SIZE               (0xFFFF)
> >
> >  /* rdma interface */
> > @@ -581,7 +572,7 @@ struct qed_roce_ll2_packet {
> >         int n_seg;
> >         struct qed_roce_ll2_buffer payload[RDMA_MAX_SGE_PER_SQ_WQE];
> >         int roce_mode;
> > -       enum qed_roce_ll2_tx_dest tx_dest;
> > +       enum qed_ll2_tx_dest tx_dest;
> >  };
> >
> >  enum qed_rdma_type {
> > --
> > 2.19.0
> >
> 
> 
> --
> Thanks,
> ~Nick Desaulniers

Thanks, definitely  looks cleaner. 
Acked-by: Michal Kalderon <michal.kalderon@cavium.com>



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] RDMA/qedr: Remove enumerated type qed_roce_ll2_tx_dest
  2018-09-27 20:55 ` [PATCH v2] RDMA/qedr: Remove enumerated type qed_roce_ll2_tx_dest Nathan Chancellor
  2018-09-27 22:29   ` Nick Desaulniers
@ 2018-09-28 16:14   ` Jason Gunthorpe
  1 sibling, 0 replies; 8+ messages in thread
From: Jason Gunthorpe @ 2018-09-28 16:14 UTC (permalink / raw)
  To: Nathan Chancellor
  Cc: Michal Kalderon, Ariel Elior, Doug Ledford, linux-rdma,
	linux-kernel, Nick Desaulniers

On Thu, Sep 27, 2018 at 01:55:58PM -0700, Nathan Chancellor 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.
> 
> Turns out that QED_ROCE_LL2_TX_DEST_NW and QED_ROCE_LL2_TX_DEST_LB are
> only used once in the whole tree and QED_ROCE_LL2_TX_DEST_MAX is used
> nowhere. Remove them and use the equivalent values from qed_ll2_tx_dest
> in their place.
> 
> Reported-by: Nick Desaulniers <ndesaulniers@google.com>
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> Acked-by: Michal Kalderon <michal.kalderon@cavium.com>
> ---
> 
> v1 -> v2:
> 
> * Rather than using an explicit cast, just convert the uses to the
>   appropriate values and delete the duplicated enum.

Applied to for-next, thanks

Jason

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2018-09-28 16:14 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-27  1:18 [PATCH] RDMA/qedr: Explicitly cast pkt->tx_dest to qed_ll2_tx_dest Nathan Chancellor
2018-09-27 20:28 ` Nick Desaulniers
2018-09-27 20:35   ` Nathan Chancellor
2018-09-27 20:41     ` Nick Desaulniers
2018-09-27 20:55 ` [PATCH v2] RDMA/qedr: Remove enumerated type qed_roce_ll2_tx_dest Nathan Chancellor
2018-09-27 22:29   ` Nick Desaulniers
2018-09-28  8:06     ` Kalderon, Michal
2018-09-28 16:14   ` Jason Gunthorpe

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).