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

[Nathan Chancellor]

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