Re: [PATCH net v2] enetc: Avoid implicit sign extension
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: 2021-03-29 16:25:05
On Mon, Mar 29, 2021 at 05:14:43PM +0300, Claudiu Manoil wrote:
Static analysis tool reports: "Suspicious implicit sign extension - 'flags' with type u8 (8 bit, unsigned) is promoted in 'flags' << 24 to type int (32 bits, signed), then sign-extended to type unsigned long long (64 bits, unsigned). If flags << 24 is greater than 0x7FFFFFFF, the upper bits of the result
This is a backwards way of saying 'if flags & BIT(7) is set', no? But BIT(7) is ENETC_TXBD_FLAGS_F (the 'final BD' bit), and I've been testing SO_TXTIME with single BD frames, and haven't seen this problem.
quoted hunk ↗ jump to hunk
will all be 1." Use lower_32_bits() to avoid this scenario. Fixes: 82728b91f124 ("enetc: Remove Tx checksumming offload code") Signed-off-by: Claudiu Manoil <claudiu.manoil@nxp.com> --- v2 - added 'fixes' tag drivers/net/ethernet/freescale/enetc/enetc_hw.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)diff --git a/drivers/net/ethernet/freescale/enetc/enetc_hw.h b/drivers/net/ethernet/freescale/enetc/enetc_hw.h index 00938f7960a4..07e03df8af94 100644 --- a/drivers/net/ethernet/freescale/enetc/enetc_hw.h +++ b/drivers/net/ethernet/freescale/enetc/enetc_hw.h@@ -535,8 +535,8 @@ static inline __le32 enetc_txbd_set_tx_start(u64 tx_start, u8 flags) { u32 temp; - temp = (tx_start >> 5 & ENETC_TXBD_TXSTART_MASK) | - (flags << ENETC_TXBD_FLAGS_OFFSET); + temp = lower_32_bits(tx_start >> 5 & ENETC_TXBD_TXSTART_MASK) | + (u32)(flags << ENETC_TXBD_FLAGS_OFFSET);
I don't actually understand why lower_32_bits called on the TX time helps, considering that the value is masked already. The static analysis tool says that the right hand side of the "|" operator is what is sign-extended: (flags << ENETC_TXBD_FLAGS_OFFSET); Isn't it sufficient that you replace "u8 flags" in the function prototype with "u32 flags"?
return cpu_to_le32(temp); } -- 2.25.1