Re: [PATCH] stmmac: avoid ipq806x constant overflow warning

[Arnd Bergmann]

On Thursday 12 November 2015 12:25:28 David Miller wrote:
> From: Arnd Bergmann 
> Date: Thu, 12 Nov 2015 15:12:48 +0100
> 
> > Building dwmac-ipq806x on a 64-bit architecture produces a harmless
> > warning from gcc:
> > 
> > stmmac/dwmac-ipq806x.c: In function 'ipq806x_gmac_probe':
> > include/linux/bitops.h:6:19: warning: overflow in implicit constant 
> > conversion [-Woverflow]
> >   val = QSGMII_PHY_CDR_EN |
> > stmmac/dwmac-ipq806x.c:333:8: note: in expansion of macro 
> > 'QSGMII_PHY_CDR_EN'
> >  #define QSGMII_PHY_CDR_EN   BIT(0)
> >  #define BIT(nr)   (1UL << (nr))
> > 
> > The compiler warns about the fact that a 64-bit literal is passed
> > into a function that takes a 32-bit argument. I could not fully understand
> > why it warns despite the fact that this number is always small enough
> > to fit, but changing the use of BIT() macros into the equivalent hexadecimal
> > representation avoids the warning
> > 
> > Signed-off-by: Arnd Bergmann 
> > Fixes: b1c17215d718 ("stmmac: add ipq806x glue layer")
> 
> I've seen this warning too on x86_64 and had been meaning to look
> into it, thanks for taking the initiative. 
> 
> Moving away from using BIT() is somewhat disappointing, because we
> want to encourage people to use these macros.

Ok, I never really liked that macro, so I didn't mind removing it. ;-)

I spent too much time working at IBM where all internal documentation
uses bit numbers that call the MSB bit 0, and drivers use randomly
either the IBM notation or the one that everyone else uses, so I always
found the hex numbers way more intuitive.

> Also I don't even understand the compiler's behavior, it's warning
> about QSGMII_PHY_CDR_EN but if you define only that to "0x1u" it still
> warns about QSGMII_PHY_CDR_EN.
> 
> The warning goes away only if you change all 5 BIT() uses.

Yes, I have spent the time to analyze the problem now and it all makes
sense. A proper patch follows.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] stmmac: avoid ipq806x constant overflow warning

[Arnd Bergmann]

Building dwmac-ipq806x on a 64-bit architecture produces a harmless
warning from gcc:

stmmac/dwmac-ipq806x.c: In function 'ipq806x_gmac_probe':
include/linux/bitops.h:6:19: warning: overflow in implicit constant conversion 
[-Woverflow]
  val = QSGMII_PHY_CDR_EN |
stmmac/dwmac-ipq806x.c:333:8: note: in expansion of macro 'QSGMII_PHY_CDR_EN'
 #define QSGMII_PHY_CDR_EN   BIT(0)
 #define BIT(nr)   (1UL << (nr))

The compiler warns about the fact that a 64-bit literal is passed
into a function that takes a 32-bit argument. I could not fully understand
why it warns despite the fact that this number is always small enough
to fit, but changing the use of BIT() macros into the equivalent hexadecimal
representation avoids the warning

Signed-off-by: Arnd Bergmann 
Fixes: b1c17215d718 ("stmmac: add ipq806x glue layer")
---
This came up on the arm64 allmodconfig build

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-ipq806x.c 
b/drivers/net/ethernet/stmicro/stmmac/dwmac-ipq806x.c
index 9d89bdbf029f..4abd9b0b542a 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-ipq806x.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-ipq806x.c
@@ -77,11 +77,11 @@
 /* Only GMAC1/2/3 support SGMII and their CTL register are not contiguous */
 #define QSGMII_PHY_SGMII_CTL(x)((x == 1) ? 0x134 : \
 (0x13c + (4 * (x - 2
-#define QSGMII_PHY_CDR_EN  BIT(0)
-#define QSGMII_PHY_RX_FRONT_EN BIT(1)
-#define QSGMII_PHY_RX_SIGNAL_DETECT_EN BIT(2)
-#define QSGMII_PHY_TX_DRIVER_ENBIT(3)
-#define QSGMII_PHY_QSGMII_EN   BIT(7)
+#define QSGMII_PHY_CDR_EN  0x01u
+#define QSGMII_PHY_RX_FRONT_EN 0x02u
+#define QSGMII_PHY_RX_SIGNAL_DETECT_EN 0x04u
+#define QSGMII_PHY_TX_DRIVER_EN0x08u
+#define QSGMII_PHY_QSGMII_EN   0x80u
 #define QSGMII_PHY_PHASE_LOOP_GAIN_OFFSET  12
 #define QSGMII_PHY_PHASE_LOOP_GAIN_MASK0x7
 #define QSGMII_PHY_RX_DC_BIAS_OFFSET   18

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] stmmac: avoid ipq806x constant overflow warning

[David Miller]

From: Arnd Bergmann 
Date: Thu, 12 Nov 2015 15:12:48 +0100

> Building dwmac-ipq806x on a 64-bit architecture produces a harmless
> warning from gcc:
> 
> stmmac/dwmac-ipq806x.c: In function 'ipq806x_gmac_probe':
> include/linux/bitops.h:6:19: warning: overflow in implicit constant 
> conversion [-Woverflow]
>   val = QSGMII_PHY_CDR_EN |
> stmmac/dwmac-ipq806x.c:333:8: note: in expansion of macro 'QSGMII_PHY_CDR_EN'
>  #define QSGMII_PHY_CDR_EN   BIT(0)
>  #define BIT(nr)   (1UL << (nr))
> 
> The compiler warns about the fact that a 64-bit literal is passed
> into a function that takes a 32-bit argument. I could not fully understand
> why it warns despite the fact that this number is always small enough
> to fit, but changing the use of BIT() macros into the equivalent hexadecimal
> representation avoids the warning
> 
> Signed-off-by: Arnd Bergmann 
> Fixes: b1c17215d718 ("stmmac: add ipq806x glue layer")

I've seen this warning too on x86_64 and had been meaning to look
into it, thanks for taking the initiative. :)

Moving away from using BIT() is somewhat disappointing, because we
want to encourage people to use these macros.

I think it's also easier from a driver author and auditing
perspective, you can see that something is being defined as bit X and
then check the documentation for the chip to see if bit X is correct
or not.

With the hex values there is more mental work and room for... mistakes.

Also I don't even understand the compiler's behavior, it's warning
about QSGMII_PHY_CDR_EN but if you define only that to "0x1u" it still
warns about QSGMII_PHY_CDR_EN.

The warning goes away only if you change all 5 BIT() uses.

This makes me like the change even less, something foul is going on
here and I'd rather figure out what that is than install this patch.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html