LinuxLists.cc - [PATCH] qed: Remove unneeded enumerated type core_tx

2018-10-04 00:09:48

Subject: [PATCH] qed: Remove unneeded enumerated type core_tx_dest

Clang warns when one enumerated type is implicitly converted to another.

drivers/net/ethernet/qlogic/qed/qed_ll2.c:799:32: warning: implicit
conversion from enumeration type 'enum core_tx_dest' to different
enumeration type 'enum qed_ll2_tx_dest' [-Wenum-conversion]
tx_pkt.tx_dest = p_ll2_conn->tx_dest;
~ ~~~~~~~~~~~~^~~~~~~
1 warning generated.

These enumerated types are not 1 to 1:

/* Light L2 TX Destination */
enum core_tx_dest {
CORE_TX_DEST_NW,
CORE_TX_DEST_LB,
CORE_TX_DEST_RESERVED,
CORE_TX_DEST_DROP,
MAX_CORE_TX_DEST
};

enum qed_ll2_tx_dest {
QED_LL2_TX_DEST_NW, /* Light L2 TX Destination to the Network */
QED_LL2_TX_DEST_LB, /* Light L2 TX Destination to the Loopback */
QED_LL2_TX_DEST_DROP, /* Light L2 Drop the TX packet */
QED_LL2_TX_DEST_MAX
};

Fix this conversion warning by adding CORE_TX_DEST_DROP to
qed_ll2_tx_dest and converting all values of core_tx_dest to
the equivalent value in qed_ll2_tx_dest so that there is no
conversion warning or functional change.

Link: https://github.com/ClangBuiltLinux/linux/issues/125
Signed-off-by: Nathan Chancellor <[email protected]>
---

I'm by no means married to the name of the new enum value, it could even
be CORE_TX_DEST_DROP (didn't know if that was going to set anyone off or
not) but this is the cleanest solution to the warning in my opinion but
I'm open to other opinions.

drivers/net/ethernet/qlogic/qed/qed_hsi.h | 9 ---------
drivers/net/ethernet/qlogic/qed/qed_ll2.c | 16 ++++++++--------
drivers/net/ethernet/qlogic/qed/qed_ll2.h | 2 +-
include/linux/qed/qed_ll2_if.h | 1 +
4 files changed, 10 insertions(+), 18 deletions(-)

diff --git a/drivers/net/ethernet/qlogic/qed/qed_hsi.h b/drivers/net/ethernet/qlogic/qed/qed_hsi.h
index 15b4cd317f14..c2ce8428c413 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_hsi.h
+++ b/drivers/net/ethernet/qlogic/qed/qed_hsi.h
@@ -331,15 +331,6 @@ struct core_tx_bd {
#define CORE_TX_BD_TX_DST_SHIFT 14
};

-/* Light L2 TX Destination */
-enum core_tx_dest {
- CORE_TX_DEST_NW,
- CORE_TX_DEST_LB,
- CORE_TX_DEST_RESERVED,
- CORE_TX_DEST_DROP,
- MAX_CORE_TX_DEST
-};
-
/* Ramrod data for tx queue start ramrod */
struct core_tx_start_ramrod_data {
struct regpair pbl_base_addr;
diff --git a/drivers/net/ethernet/qlogic/qed/qed_ll2.c b/drivers/net/ethernet/qlogic/qed/qed_ll2.c
index da13117a604a..741185a62e84 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_ll2.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_ll2.c
@@ -1345,13 +1345,13 @@ int qed_ll2_acquire_connection(void *cxt, struct qed_ll2_acquire_data *data)

switch (data->input.tx_dest) {
case QED_LL2_TX_DEST_NW:
- p_ll2_info->tx_dest = CORE_TX_DEST_NW;
+ p_ll2_info->tx_dest = QED_LL2_TX_DEST_NW;
break;
case QED_LL2_TX_DEST_LB:
- p_ll2_info->tx_dest = CORE_TX_DEST_LB;
+ p_ll2_info->tx_dest = QED_LL2_TX_DEST_LB;
break;
case QED_LL2_TX_DEST_DROP:
- p_ll2_info->tx_dest = CORE_TX_DEST_DROP;
+ p_ll2_info->tx_dest = QED_LL2_TX_DEST_DROP_CORE;
break;
default:
return -EINVAL;
@@ -1684,7 +1684,7 @@ qed_ll2_prepare_tx_packet_set_bd(struct qed_hwfn *p_hwfn,
u16 prod_idx = qed_chain_get_prod_idx(p_tx_chain);
struct core_tx_bd *start_bd = NULL;
enum core_roce_flavor_type roce_flavor;
- enum core_tx_dest tx_dest;
+ enum qed_ll2_tx_dest tx_dest;
u16 bd_data = 0, frag_idx;

roce_flavor = (pkt->qed_roce_flavor == QED_LL2_ROCE) ? CORE_ROCE
@@ -1692,16 +1692,16 @@ qed_ll2_prepare_tx_packet_set_bd(struct qed_hwfn *p_hwfn,

switch (pkt->tx_dest) {
case QED_LL2_TX_DEST_NW:
- tx_dest = CORE_TX_DEST_NW;
+ tx_dest = QED_LL2_TX_DEST_NW;
break;
case QED_LL2_TX_DEST_LB:
- tx_dest = CORE_TX_DEST_LB;
+ tx_dest = QED_LL2_TX_DEST_LB;
break;
case QED_LL2_TX_DEST_DROP:
- tx_dest = CORE_TX_DEST_DROP;
+ tx_dest = QED_LL2_TX_DEST_DROP_CORE;
break;
default:
- tx_dest = CORE_TX_DEST_LB;
+ tx_dest = QED_LL2_TX_DEST_LB;
break;
}

diff --git a/drivers/net/ethernet/qlogic/qed/qed_ll2.h b/drivers/net/ethernet/qlogic/qed/qed_ll2.h
index 1a5c1ae01474..e676bbaf55c9 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_ll2.h
+++ b/drivers/net/ethernet/qlogic/qed/qed_ll2.h
@@ -119,7 +119,7 @@ struct qed_ll2_info {
u8 queue_id;
u8 tx_stats_id;
bool b_active;
- enum core_tx_dest tx_dest;
+ enum qed_ll2_tx_dest tx_dest;
u8 tx_stats_en;
bool main_func_queue;
struct qed_ll2_rx_queue rx_queue;
diff --git a/include/linux/qed/qed_ll2_if.h b/include/linux/qed/qed_ll2_if.h
index 5eb022953aca..1b260e73943c 100644
--- a/include/linux/qed/qed_ll2_if.h
+++ b/include/linux/qed/qed_ll2_if.h
@@ -65,6 +65,7 @@ enum qed_ll2_tx_dest {
QED_LL2_TX_DEST_NW, /* Light L2 TX Destination to the Network */
QED_LL2_TX_DEST_LB, /* Light L2 TX Destination to the Loopback */
QED_LL2_TX_DEST_DROP, /* Light L2 Drop the TX packet */
+ QED_LL2_TX_DEST_DROP_CORE, /* CORE_TX_DEST_DROP value */
QED_LL2_TX_DEST_MAX
};

--
2.19.0

2018-10-04 16:24:10

by Tayar, Tomer

[permalink] [raw]

Subject: RE: [PATCH] qed: Remove unneeded enumerated type core_tx_dest

From: Nathan Chancellor <[email protected]>
Sent: Thursday, October 04, 2018 3:09 AM

> Clang warns when one enumerated type is implicitly converted to another.
>
> drivers/net/ethernet/qlogic/qed/qed_ll2.c:799:32: warning: implicit
> conversion from enumeration type 'enum core_tx_dest' to different
> enumeration type 'enum qed_ll2_tx_dest' [-Wenum-conversion]
> tx_pkt.tx_dest = p_ll2_conn->tx_dest;
> ~ ~~~~~~~~~~~~^~~~~~~
> 1 warning generated.
>
> These enumerated types are not 1 to 1:
>
> /* Light L2 TX Destination */
> enum core_tx_dest {
> CORE_TX_DEST_NW,
> CORE_TX_DEST_LB,
> CORE_TX_DEST_RESERVED,
> CORE_TX_DEST_DROP,
> MAX_CORE_TX_DEST
> };
>
> enum qed_ll2_tx_dest {
> QED_LL2_TX_DEST_NW, /* Light L2 TX Destination to the Network */
> QED_LL2_TX_DEST_LB, /* Light L2 TX Destination to the Loopback */
> QED_LL2_TX_DEST_DROP, /* Light L2 Drop the TX packet */
> QED_LL2_TX_DEST_MAX
> };
>
> Fix this conversion warning by adding CORE_TX_DEST_DROP to
> qed_ll2_tx_dest and converting all values of core_tx_dest to
> the equivalent value in qed_ll2_tx_dest so that there is no
> conversion warning or functional change.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/125
> Signed-off-by: Nathan Chancellor <[email protected]>

[...]

> -/* Light L2 TX Destination */
> -enum core_tx_dest {
> - CORE_TX_DEST_NW,
> - CORE_TX_DEST_LB,
> - CORE_TX_DEST_RESERVED,
> - CORE_TX_DEST_DROP,
> - MAX_CORE_TX_DEST
> -};

[...]

> QED_LL2_TX_DEST_NW, /* Light L2 TX Destination to the Network */
> QED_LL2_TX_DEST_LB, /* Light L2 TX Destination to the Loopback */
> QED_LL2_TX_DEST_DROP, /* Light L2 Drop the TX packet */
> + QED_LL2_TX_DEST_DROP_CORE, /* CORE_TX_DEST_DROP value */
> QED_LL2_TX_DEST_MAX
> };

Thanks Nathan for finding this issue.
"enum core_tx_dest" is for the interface with the device FW, while "enum qed_ll2_tx_dest" is for the interface with other qed* kernel modules.
The distinction is on purpose, so "enum core_tx_dest" shouldn't be deleted.
Maybe an explicit switch/case would be better as a fix?
E.g. -
- tx_pkt.tx_dest = p_ll2_conn->tx_dest;
+ switch (p_ll2_conn->tx_dest) {
+ case CORE_TX_DEST_NW:
+ tx_pkt.tx_dest = QED_LL2_TX_DEST_NW;
+ break;
+ case CORE_TX_DEST_LB:
+ tx_pkt.tx_dest = QED_LL2_TX_DEST_LB;
+ break;
+ case CORE_TX_DEST_DROP:
+ tx_pkt.tx_dest = QED_LL2_TX_DEST_DROP;
+ break;
+ default:
+ tx_pkt.tx_dest = QED_LL2_TX_DEST_DROP;
+ }

2018-10-04 16:28:03

by Nathan Chancellor

[permalink] [raw]

Subject: Re: [PATCH] qed: Remove unneeded enumerated type core_tx_dest

On Thu, Oct 04, 2018 at 04:23:43PM +0000, Tayar, Tomer wrote:
> From: Nathan Chancellor <[email protected]>
> Sent: Thursday, October 04, 2018 3:09 AM
>
> > Clang warns when one enumerated type is implicitly converted to another.
> >
> > drivers/net/ethernet/qlogic/qed/qed_ll2.c:799:32: warning: implicit
> > conversion from enumeration type 'enum core_tx_dest' to different
> > enumeration type 'enum qed_ll2_tx_dest' [-Wenum-conversion]
> > tx_pkt.tx_dest = p_ll2_conn->tx_dest;
> > ~ ~~~~~~~~~~~~^~~~~~~
> > 1 warning generated.
> >
> > These enumerated types are not 1 to 1:
> >
> > /* Light L2 TX Destination */
> > enum core_tx_dest {
> > CORE_TX_DEST_NW,
> > CORE_TX_DEST_LB,
> > CORE_TX_DEST_RESERVED,
> > CORE_TX_DEST_DROP,
> > MAX_CORE_TX_DEST
> > };
> >
> > enum qed_ll2_tx_dest {
> > QED_LL2_TX_DEST_NW, /* Light L2 TX Destination to the Network */
> > QED_LL2_TX_DEST_LB, /* Light L2 TX Destination to the Loopback */
> > QED_LL2_TX_DEST_DROP, /* Light L2 Drop the TX packet */
> > QED_LL2_TX_DEST_MAX
> > };
> >
> > Fix this conversion warning by adding CORE_TX_DEST_DROP to
> > qed_ll2_tx_dest and converting all values of core_tx_dest to
> > the equivalent value in qed_ll2_tx_dest so that there is no
> > conversion warning or functional change.
> >
> > Link: https://github.com/ClangBuiltLinux/linux/issues/125
> > Signed-off-by: Nathan Chancellor <[email protected]>
>
> [...]
>
> > -/* Light L2 TX Destination */
> > -enum core_tx_dest {
> > - CORE_TX_DEST_NW,
> > - CORE_TX_DEST_LB,
> > - CORE_TX_DEST_RESERVED,
> > - CORE_TX_DEST_DROP,
> > - MAX_CORE_TX_DEST
> > -};
>
> [...]
>
> > QED_LL2_TX_DEST_NW, /* Light L2 TX Destination to the Network */
> > QED_LL2_TX_DEST_LB, /* Light L2 TX Destination to the Loopback */
> > QED_LL2_TX_DEST_DROP, /* Light L2 Drop the TX packet */
> > + QED_LL2_TX_DEST_DROP_CORE, /* CORE_TX_DEST_DROP value */
> > QED_LL2_TX_DEST_MAX
> > };
>
> Thanks Nathan for finding this issue.
> "enum core_tx_dest" is for the interface with the device FW, while "enum qed_ll2_tx_dest" is for the interface with other qed* kernel modules.
> The distinction is on purpose, so "enum core_tx_dest" shouldn't be deleted.
> Maybe an explicit switch/case would be better as a fix?
> E.g. -
> - tx_pkt.tx_dest = p_ll2_conn->tx_dest;
> + switch (p_ll2_conn->tx_dest) {
> + case CORE_TX_DEST_NW:
> + tx_pkt.tx_dest = QED_LL2_TX_DEST_NW;
> + break;
> + case CORE_TX_DEST_LB:
> + tx_pkt.tx_dest = QED_LL2_TX_DEST_LB;
> + break;
> + case CORE_TX_DEST_DROP:
> + tx_pkt.tx_dest = QED_LL2_TX_DEST_DROP;
> + break;
> + default:
> + tx_pkt.tx_dest = QED_LL2_TX_DEST_DROP;
> + }
>

Hi Tomer,

Yes, that should work and it would match the rest of the driver's
handling of this distinction. I will go ahead and spin up a v2 here
shortly for review, thank you.

Nathan

2018-10-04 16:41:24

by Nathan Chancellor

[permalink] [raw]

Subject: [PATCH v2] qed: Avoid implicit enum conversion in qed_ooo_submit_tx_buffers

2018-10-04 16:46:57

by Tayar, Tomer

[permalink] [raw]

Subject: RE: [PATCH v2] qed: Avoid implicit enum conversion in qed_ooo_submit_tx_buffers

From: Nathan Chancellor <[email protected]>
Sent: Thursday, October 04, 2018 7:39 PM

> Clang warns when one enumerated type is implicitly converted to another.
>
> drivers/net/ethernet/qlogic/qed/qed_ll2.c:799:32: warning: implicit
> conversion from enumeration type 'enum core_tx_dest' to different
> enumeration type 'enum qed_ll2_tx_dest' [-Wenum-conversion]
> tx_pkt.tx_dest = p_ll2_conn->tx_dest;
> ~ ~~~~~~~~~~~~^~~~~~~
> 1 warning generated.
>
> Fix this by using a switch statement to convert between the enumerated
> values since they are not 1 to 1, which matches how the rest of the
> driver handles this conversion.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/125
> Suggested-by: Tomer Tayar <[email protected]>
> Signed-off-by: Nathan Chancellor <[email protected]>
> ---
>
> v1 -> v2:
>
> * Use an explicit switch statement to convert between the enumerated
> types like the rest of the driver, as suggested by Tomer.

Thanks
Acked-by: Tomer Tayar <[email protected]>

2018-10-04 17:06:01

by David Miller

[permalink] [raw]

Subject: Re: [PATCH v2] qed: Avoid implicit enum conversion in qed_ooo_submit_tx_buffers

From: Nathan Chancellor <[email protected]>
Date: Thu, 4 Oct 2018 09:39:20 -0700

> Clang warns when one enumerated type is implicitly converted to another.
>
> drivers/net/ethernet/qlogic/qed/qed_ll2.c:799:32: warning: implicit
> conversion from enumeration type 'enum core_tx_dest' to different
> enumeration type 'enum qed_ll2_tx_dest' [-Wenum-conversion]
> tx_pkt.tx_dest = p_ll2_conn->tx_dest;
> ~ ~~~~~~~~~~~~^~~~~~~
> 1 warning generated.
>
> Fix this by using a switch statement to convert between the enumerated
> values since they are not 1 to 1, which matches how the rest of the
> driver handles this conversion.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/125
> Suggested-by: Tomer Tayar <[email protected]>
> Signed-off-by: Nathan Chancellor <[email protected]>
> ---
>
> v1 -> v2:
>
> * Use an explicit switch statement to convert between the enumerated
> types like the rest of the driver, as suggested by Tomer.

Applied to net-next.