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

From: Nathan Chancellor
Date: Thu Oct 04 2018 - 12:27:43 EST


On Thu, Oct 04, 2018 at 04:23:43PM +0000, Tayar, Tomer wrote:
> From: Nathan Chancellor <natechancellor@xxxxxxxxx>
> 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 <natechancellor@xxxxxxxxx>
>
> [...]
>
> > -/* 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