| Message ID | 20180808075928.31107-2-wsa+renesas@sang-engineering.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | i2c: rcar: implement STOP and REP_START according to docs | expand |
> On August 8, 2018 at 9:59 AM Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > > > Use BIT macro to avoid shift-31-problem, indent a little more and use > GENMASK to make it easier to add new flags. > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > drivers/i2c/busses/i2c-rcar.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c > index 791a4aa34fdd..a9f1880e2eae 100644 > --- a/drivers/i2c/busses/i2c-rcar.c > +++ b/drivers/i2c/busses/i2c-rcar.c > @@ -19,6 +19,7 @@ > * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > * GNU General Public License for more details. > */ > +#include <linux/bitops.h> > #include <linux/clk.h> > #include <linux/delay.h> > #include <linux/dmaengine.h> > @@ -112,9 +113,9 @@ > #define ID_ARBLOST (1 << 3) > #define ID_NACK (1 << 4) > /* persistent flags */ > -#define ID_P_NO_RXDMA (1 << 30) /* HW forbids RXDMA sometimes */ > -#define ID_P_PM_BLOCKED (1 << 31) > -#define ID_P_MASK (ID_P_PM_BLOCKED | ID_P_NO_RXDMA) > +#define ID_P_NO_RXDMA BIT(30) /* HW forbids RXDMA sometimes */ > +#define ID_P_PM_BLOCKED BIT(31) > +#define ID_P_MASK GENMASK(31, 30) > > enum rcar_i2c_type { > I2C_RCAR_GEN1, > -- > 2.11.0 > Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu> CU Uli
On Wed, Aug 08, 2018 at 09:59:27AM +0200, Wolfram Sang wrote: > Use BIT macro to avoid shift-31-problem, indent a little more and use > GENMASK to make it easier to add new flags. > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> Applied to for-next, thanks!
Hi Wolfram, > On August 8, 2018 at 9:59 AM Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > > Use BIT macro to avoid shift-31-problem, May I ask how exactly you spotted the "shift-31-problem" in drivers/i2c/busses/i2c-rcar.c: - visual code review? - static analysis, special compiler flags? - run-time testing with UBSAN=y? The background of my question is seeing a lot of UBSAN errors caused by (1 << 31) in U-Boot with UBSAN=y [1]. Based on my experiments, the Linux UBSAN simply doesn't complain about (1 << 31). I traced the difference to be caused by the -std=gnu{89,11} value passed to gcc, which varies between U-Boot and Linux. According to feedback from GCC community [2], with 'gcc -std=gnu89', shifting into (not past) the sign bit is "defined behavior" which is why UBSAN doesn't report this as an issue in Linux kernel. That makes me curious about the background of this patch, since it might shed some light onto how to further handle the (1 << 31) UBSAN warnings in U-Boot. Thank you very much for your feedback. [1] https://patchwork.ozlabs.org/cover/962307/ [2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87392 Best regards, Eugeniu.
> May I ask how exactly you spotted the "shift-31-problem" in > drivers/i2c/busses/i2c-rcar.c: > - visual code review? > - static analysis, special compiler flags? This one. I run a set of static code analyziers when applying patches. One of them is 'cppcheck' which reported it. > According to feedback from GCC community [2], with 'gcc -std=gnu89', > shifting into (not past) the sign bit is "defined behavior" which is why > UBSAN doesn't report this as an issue in Linux kernel. That makes me I see. I guess it can be argued. Yet, BIT() solves other issues as well ('1' vs '1u'), so this was probably a reasonable move nonetheless, plus we are super-super-sure about the shifting now.
Hi Wolfram, On Fri, Oct 05, 2018 at 06:27:28PM +0200, Wolfram Sang wrote: > > > May I ask how exactly you spotted the "shift-31-problem" in > > drivers/i2c/busses/i2c-rcar.c: > > - visual code review? > > - static analysis, special compiler flags? > > This one. I run a set of static code analyziers when applying patches. > One of them is 'cppcheck' which reported it. Indeed, cppcheck reports w/o this patch: [drivers/i2c/busses/i2c-rcar.c:972]: (error) Shifting signed 32-bit value by 31 bits is undefined behaviour [drivers/i2c/busses/i2c-rcar.c:1008]: (error) Shifting signed 32-bit value by 31 bits is undefined behaviour > > > According to feedback from GCC community [2], with 'gcc -std=gnu89', > > shifting into (not past) the sign bit is "defined behavior" which is why > > UBSAN doesn't report this as an issue in Linux kernel. That makes me > > I see. I guess it can be argued. Yet, BIT() solves other issues as well > ('1' vs '1u'), so this was probably a reasonable move nonetheless, plus > we are super-super-sure about the shifting now. > I agree. There is no doubt that avoiding/fixing shifting into the sign bit makes the code more portable and will lessen the pain when switching Kbuild to C99/C11 (if ever needed). I still have open questions, but since they go beyond i2c framework and beyond kernel itself (as said, they originate from porting UBSan to U-Boot), I will discuss them elsewhere. Thanks again for the reply. Best regards, Eugeniu.
diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c index 791a4aa34fdd..a9f1880e2eae 100644 --- a/drivers/i2c/busses/i2c-rcar.c +++ b/drivers/i2c/busses/i2c-rcar.c @@ -19,6 +19,7 @@ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the * GNU General Public License for more details. */ +#include <linux/bitops.h> #include <linux/clk.h> #include <linux/delay.h> #include <linux/dmaengine.h> @@ -112,9 +113,9 @@ #define ID_ARBLOST (1 << 3) #define ID_NACK (1 << 4) /* persistent flags */ -#define ID_P_NO_RXDMA (1 << 30) /* HW forbids RXDMA sometimes */ -#define ID_P_PM_BLOCKED (1 << 31) -#define ID_P_MASK (ID_P_PM_BLOCKED | ID_P_NO_RXDMA) +#define ID_P_NO_RXDMA BIT(30) /* HW forbids RXDMA sometimes */ +#define ID_P_PM_BLOCKED BIT(31) +#define ID_P_MASK GENMASK(31, 30) enum rcar_i2c_type { I2C_RCAR_GEN1,
Use BIT macro to avoid shift-31-problem, indent a little more and use GENMASK to make it easier to add new flags. Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> --- drivers/i2c/busses/i2c-rcar.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)