netdev - Re: [PATCH] qed: Remove unneeded enumerated type core_tx

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 4 Oct 2018 09:27:36 -0700
From:   Nathan Chancellor <natechancellor@...il.com>
To:     "Tayar, Tomer" <Tomer.Tayar@...ium.com>
Cc:     "Elior, Ariel" <Ariel.Elior@...ium.com>,
        Dept-Eng Everest Linux L2 <Dept-EngEverestLinuxL2@...ium.com>,
        "David S. Miller" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Nick Desaulniers <ndesaulniers@...gle.com>
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 <natechancellor@...il.com>
> 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@...il.com>
> 
> [...]
> 
> > -/* 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