Skip to contentSkip to conversation

treewide: mask then shift defects and style updates

PATCH Applied Joe Perches 14 participants 31 messages Oct 27, 2014 – Nov 10, 2014

Subsystems (18)
Networking Network drivers DRM drivers Media (V4L/DVB) +14 more subsystems
lore
Share & tools
lore

Joe Perches

author
#0 lore raw

cover treewide: mask then shift defects and style updates

logical mask has lower precedence than shift but should be done before the shift so parentheses are generally required.

And when masking with a fixed value after a shift, normal kernel style has the shift on the left, then the shift on the right so convert a few non-conforming uses.

Joe Perches (11):
block: nvme-scsi: Fix probable mask then right shift defects
radeon: evergreen: Fix probable mask then right shift defects
aiptek: Fix probable mask then right shift defects
dvb-net: Fix probable mask then right shift defects cx25840/cx18: Use standard ordering of mask and shift
wm8350-core: Fix probable mask then right shift defect
iwlwifi: dvm: Fix probable mask then right shift defect
ssb: driver_chip_comon_pmu: Fix probable mask then right shift defect
tty: ipwireless: Fix probable mask then right shift defects
hwa-hc: Fix probable mask then right shift defect
sound: ad1889: Fix probable mask then right shift defects

 drivers/block/nvme-scsi.c                | 12 ++++++------
 drivers/gpu/drm/radeon/evergreen.c       |  3 ++-
 drivers/input/tablet/aiptek.c            |  6 +++---
 drivers/media/dvb-core/dvb_net.c         |  4 +++-
 drivers/media/i2c/cx25840/cx25840-core.c | 12 ++++++------
 drivers/media/pci/cx18/cx18-av-core.c    | 16 ++++++++--------
 drivers/mfd/wm8350-core.c                |  2 +-
 drivers/net/wireless/iwlwifi/dvm/lib.c   |  4 ++--
 drivers/ssb/driver_chipcommon_pmu.c      |  4 ++--
 drivers/tty/ipwireless/hardware.c        | 12 ++++++------
 drivers/usb/host/hwa-hc.c                |  2 +-
 sound/pci/ad1889.c                       |  8 ++++----
 12 files changed, 44 insertions(+), 41 deletions(-)
Signature · 1 line
2.1.2
author
#1 lore raw

PATCH 1/11 block: nvme-scsi: Fix probable mask then right shift defects

Precedence of & and >> is not the same and is not left to right. shift has higher precedence and should be done after the mask.

Add parentheses around the mask.

Signed-off-by Joe Perches <joe at perches.com>
---
 drivers/block/nvme-scsi.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
drivers/block/nvme-scsi.c +6 -6
index a4cd6d6..529ee54 100644
@@ -1972,8 +1972,8 @@ static inline void nvme_trans_get_io_cdb10(u8 *cmd,
 {
 	cdb_info->fua = GET_U8_FROM_CDB(cmd, IO_10_CDB_FUA_OFFSET) &
 					IO_CDB_FUA_MASK;
-	cdb_info->prot_info = GET_U8_FROM_CDB(cmd, IO_10_CDB_WP_OFFSET) &
-					IO_CDB_WP_MASK >> IO_CDB_WP_SHIFT;
+	cdb_info->prot_info = (GET_U8_FROM_CDB(cmd, IO_10_CDB_WP_OFFSET) &
+			       IO_CDB_WP_MASK) >> IO_CDB_WP_SHIFT;
 	cdb_info->lba = GET_U32_FROM_CDB(cmd, IO_10_CDB_LBA_OFFSET);
 	cdb_info->xfer_len = GET_U16_FROM_CDB(cmd, IO_10_CDB_TX_LEN_OFFSET);
 }

@@ -1983,8 +1983,8 @@ static inline void nvme_trans_get_io_cdb12(u8 *cmd,
 {
 	cdb_info->fua = GET_U8_FROM_CDB(cmd, IO_12_CDB_FUA_OFFSET) &
 					IO_CDB_FUA_MASK;
-	cdb_info->prot_info = GET_U8_FROM_CDB(cmd, IO_12_CDB_WP_OFFSET) &
-					IO_CDB_WP_MASK >> IO_CDB_WP_SHIFT;
+	cdb_info->prot_info = (GET_U8_FROM_CDB(cmd, IO_12_CDB_WP_OFFSET) &
+			       IO_CDB_WP_MASK) >> IO_CDB_WP_SHIFT;
 	cdb_info->lba = GET_U32_FROM_CDB(cmd, IO_12_CDB_LBA_OFFSET);
 	cdb_info->xfer_len = GET_U32_FROM_CDB(cmd, IO_12_CDB_TX_LEN_OFFSET);
 }

@@ -1994,8 +1994,8 @@ static inline void nvme_trans_get_io_cdb16(u8 *cmd,
 {
 	cdb_info->fua = GET_U8_FROM_CDB(cmd, IO_16_CDB_FUA_OFFSET) &
 					IO_CDB_FUA_MASK;
-	cdb_info->prot_info = GET_U8_FROM_CDB(cmd, IO_16_CDB_WP_OFFSET) &
-					IO_CDB_WP_MASK >> IO_CDB_WP_SHIFT;
+	cdb_info->prot_info = (GET_U8_FROM_CDB(cmd, IO_16_CDB_WP_OFFSET) &
+			       IO_CDB_WP_MASK) >> IO_CDB_WP_SHIFT;
 	cdb_info->lba = GET_U64_FROM_CDB(cmd, IO_16_CDB_LBA_OFFSET);
 	cdb_info->xfer_len = GET_U32_FROM_CDB(cmd, IO_16_CDB_TX_LEN_OFFSET);
 }
Signature · 1 line
2.1.2
Acked-by Vishal Verma on v1 1/11
#2 lore raw

Re: [PATCH 01/11] block: nvme-scsi: Fix probable mask then right shift defects

Quoted 6 lines · Joe Perches

Precedence of & and >> is not the same and is not left to right. shift has higher precedence and should be done after the mask.

Add parentheses around the mask.

Signed-off-by Joe Perches <joe at perches.com>
Acked-by Vishal Verma <vishal.l.verma at linux.intel.com>
Quoted 41 lines
---
 drivers/block/nvme-scsi.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
drivers/block/nvme-scsi.c +6 -6
index a4cd6d6..529ee54 100644
@@ -1972,8 +1972,8 @@ static inline void nvme_trans_get_io_cdb10(u8 *cmd,
 {
 	cdb_info->fua = GET_U8_FROM_CDB(cmd, IO_10_CDB_FUA_OFFSET) &
 					IO_CDB_FUA_MASK;
-	cdb_info->prot_info = GET_U8_FROM_CDB(cmd, IO_10_CDB_WP_OFFSET) &
-					IO_CDB_WP_MASK >> IO_CDB_WP_SHIFT;
+	cdb_info->prot_info = (GET_U8_FROM_CDB(cmd, IO_10_CDB_WP_OFFSET) &
+			       IO_CDB_WP_MASK) >> IO_CDB_WP_SHIFT;
 	cdb_info->lba = GET_U32_FROM_CDB(cmd, IO_10_CDB_LBA_OFFSET);
 	cdb_info->xfer_len = GET_U16_FROM_CDB(cmd, IO_10_CDB_TX_LEN_OFFSET);
 }

@@ -1983,8 +1983,8 @@ static inline void nvme_trans_get_io_cdb12(u8 *cmd,
 {
 	cdb_info->fua = GET_U8_FROM_CDB(cmd, IO_12_CDB_FUA_OFFSET) &
 					IO_CDB_FUA_MASK;
-	cdb_info->prot_info = GET_U8_FROM_CDB(cmd, IO_12_CDB_WP_OFFSET) &
-					IO_CDB_WP_MASK >> IO_CDB_WP_SHIFT;
+	cdb_info->prot_info = (GET_U8_FROM_CDB(cmd, IO_12_CDB_WP_OFFSET) &
+			       IO_CDB_WP_MASK) >> IO_CDB_WP_SHIFT;
 	cdb_info->lba = GET_U32_FROM_CDB(cmd, IO_12_CDB_LBA_OFFSET);
 	cdb_info->xfer_len = GET_U32_FROM_CDB(cmd, IO_12_CDB_TX_LEN_OFFSET);
 }

@@ -1994,8 +1994,8 @@ static inline void nvme_trans_get_io_cdb16(u8 *cmd,
 {
 	cdb_info->fua = GET_U8_FROM_CDB(cmd, IO_16_CDB_FUA_OFFSET) &
 					IO_CDB_FUA_MASK;
-	cdb_info->prot_info = GET_U8_FROM_CDB(cmd, IO_16_CDB_WP_OFFSET) &
-					IO_CDB_WP_MASK >> IO_CDB_WP_SHIFT;
+	cdb_info->prot_info = (GET_U8_FROM_CDB(cmd, IO_16_CDB_WP_OFFSET) &
+			       IO_CDB_WP_MASK) >> IO_CDB_WP_SHIFT;
 	cdb_info->lba = GET_U64_FROM_CDB(cmd, IO_16_CDB_LBA_OFFSET);
 	cdb_info->xfer_len = GET_U32_FROM_CDB(cmd, IO_16_CDB_TX_LEN_OFFSET);
 }
author
#3 lore raw

PATCH 2/11 radeon: evergreen: Fix probable mask then right shift defects

In reply to Joe Perches

Precedence of & and >> is not the same and is not left to right. shift has higher precedence and should be done after the mask.

Add parentheses around the mask.

Use the already #defined values instead of hardcoding.

Signed-off-by Joe Perches <[email protected]>
---
 drivers/gpu/drm/radeon/evergreen.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/gpu/drm/radeon/evergreen.c +2 -1
index a31f1ca..a97a685 100644
@@ -3303,7 +3303,8 @@ static void evergreen_gpu_init(struct radeon_device *rdev)
 	rdev->config.evergreen.tile_config |=
 		((gb_addr_config & 0x30000000) >> 28) << 12;
 
-	num_shader_engines = (gb_addr_config & NUM_SHADER_ENGINES(3) >> 12) + 1;
+	num_shader_engines = ((gb_addr_config & NUM_SHADER_ENGINES_MASK)
+			      >> NUM_SHADER_ENGINES) + 1;
 
 	if ((rdev->family >= CHIP_CEDAR) && (rdev->family <= CHIP_HEMLOCK)) {
 		u32 efuse_straps_4;
Signature · 1 line
2.1.2
#4 lore raw

Re: [PATCH 02/11] radeon: evergreen: Fix probable mask then right shift defects

Quoted 17 lines · Joe Perches

Precedence of & and >> is not the same and is not left to right. shift has higher precedence and should be done after the mask.

Add parentheses around the mask.

Use the already #defined values instead of hardcoding.

Signed-off-by Joe Perches <[email protected]>
---
  drivers/gpu/drm/radeon/evergreen.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)
drivers/gpu/drm/radeon/evergreen.c +0 -0
index a31f1ca..a97a685 100644

drivers/gpu/drm/radeon/evergreen.c static void evergreen_gpu_init(struct radeon_device *rdev) · line 3307

Quoted 6 lines
drivers/gpu/drm/radeon/evergreen.c +2 -1
@@ -3303,7 +3303,8 @@ static void evergreen_gpu_init(struct radeon_device *rdev)
  	rdev->config.evergreen.tile_config |=
  		((gb_addr_config & 0x30000000) >> 28) << 12;
 
-	num_shader_engines = (gb_addr_config & NUM_SHADER_ENGINES(3) >> 12) + 1;
+	num_shader_engines = ((gb_addr_config & NUM_SHADER_ENGINES_MASK)
+			      >> NUM_SHADER_ENGINES) + 1;

                                 ^^^^^^^^^^^^^^^^^^

I think this should be NUM_SHADER_ENGINES_SHIFT?

Looks good to me other than that.

Signature · 6 lines
Earthling Michel Dänzer            |                  http://www.amd.com
Libre software enthusiast          |                Mesa and X developer
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
author
#5 lore raw

PATCH v2 2/11 radeon: evergreen: Fix probable mask then right shift defect

Precedence of & and >> is not the same and is not left to right. shift has higher precedence and should be done after the mask.

Add parentheses around the mask.

Use the already #defined values instead of hardcoding.

Signed-off-by Joe Perches <[email protected]>

---

Quoted 1 line

I think this should be NUM_SHADER_ENGINES_SHIFT?

(Joe can't type)

exactly right, thanks Michel

 drivers/gpu/drm/radeon/evergreen.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/gpu/drm/radeon/evergreen.c +2 -1
index a31f1ca..a97a685 100644
@@ -3303,7 +3303,8 @@ static void evergreen_gpu_init(struct radeon_device *rdev)
 	rdev->config.evergreen.tile_config |=
 		((gb_addr_config & 0x30000000) >> 28) << 12;
 
-	num_shader_engines = (gb_addr_config & NUM_SHADER_ENGINES(3) >> 12) + 1;
+	num_shader_engines = ((gb_addr_config & NUM_SHADER_ENGINES_MASK)
+			      >> NUM_SHADER_ENGINES_SHIFT) + 1;
 
 	if ((rdev->family >= CHIP_CEDAR) && (rdev->family <= CHIP_HEMLOCK)) {
 		u32 efuse_straps_4;

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Reviewed-by on v2 2/11
#6 lore raw

Re: [PATCH 02/11 V2] radeon: evergreen: Fix probable mask then right shift defect

Quoted 23 lines · Joe Perches

Precedence of & and >> is not the same and is not left to right. shift has higher precedence and should be done after the mask.

Add parentheses around the mask.

Use the already #defined values instead of hardcoding.

Signed-off-by Joe Perches <[email protected]>

---

Quoted 1 line

I think this should be NUM_SHADER_ENGINES_SHIFT?

(Joe can't type)

exactly right, thanks Michel

  drivers/gpu/drm/radeon/evergreen.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)
drivers/gpu/drm/radeon/evergreen.c +0 -0
index a31f1ca..a97a685 100644

drivers/gpu/drm/radeon/evergreen.c static void evergreen_gpu_init(struct radeon_device *rdev) · line 3310

Quoted 9 lines
drivers/gpu/drm/radeon/evergreen.c +2 -1
@@ -3303,7 +3303,8 @@ static void evergreen_gpu_init(struct radeon_device *rdev)
  	rdev->config.evergreen.tile_config |=
  		((gb_addr_config & 0x30000000) >> 28) << 12;
 
-	num_shader_engines = (gb_addr_config & NUM_SHADER_ENGINES(3) >> 12) + 1;
+	num_shader_engines = ((gb_addr_config & NUM_SHADER_ENGINES_MASK)
+			      >> NUM_SHADER_ENGINES_SHIFT) + 1;
 
  	if ((rdev->family >= CHIP_CEDAR) && (rdev->family <= CHIP_HEMLOCK)) {
  		u32 efuse_straps_4;
Reviewed-by Michel Dänzer <[email protected]>
Signature · 6 lines
Earthling Michel Dänzer            |                  http://www.amd.com
Libre software enthusiast          |                Mesa and X developer
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
#7 lore raw

Re: [PATCH 02/11 V2] radeon: evergreen: Fix probable mask then right shift defect

In reply to Joe Perches
Quoted 7 lines · Joe Perches

Precedence of & and >> is not the same and is not left to right. shift has higher precedence and should be done after the mask.

Add parentheses around the mask.

Use the already #defined values instead of hardcoding.

Quoted 7 lines
Signed-off-by Joe Perches <[email protected]>

---

Quoted 1 line

I think this should be NUM_SHADER_ENGINES_SHIFT?

(Joe can't type)

exactly right, thanks Michel

Applied with a compile fix.

Thanks,

Alex

Quoted 20 lines
 drivers/gpu/drm/radeon/evergreen.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/gpu/drm/radeon/evergreen.c +2 -1
index a31f1ca..a97a685 100644
@@ -3303,7 +3303,8 @@ static void evergreen_gpu_init(struct radeon_device *rdev)
        rdev->config.evergreen.tile_config |=
                ((gb_addr_config & 0x30000000) >> 28) << 12;
 
-       num_shader_engines = (gb_addr_config & NUM_SHADER_ENGINES(3) >> 12) + 1;
+       num_shader_engines = ((gb_addr_config & NUM_SHADER_ENGINES_MASK)
+                             >> NUM_SHADER_ENGINES_SHIFT) + 1;
 
        if ((rdev->family >= CHIP_CEDAR) && (rdev->family <= CHIP_HEMLOCK)) {
                u32 efuse_straps_4;
Quoted 4 lines

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

#8 lore raw

Re: [PATCH 02/11 V2] radeon: evergreen: Fix probable mask then right shift defect

Quoted 10 lines · Alex Deucher
Quoted 9 lines · Joe Perches

Precedence of & and >> is not the same and is not left to right. shift has higher precedence and should be done after the mask.

Add parentheses around the mask.

Use the already #defined values instead of hardcoding.

Signed-off-by Joe Perches <[email protected]>

---

Quoted 7 lines
Quoted 5 lines
Quoted 1 line

I think this should be NUM_SHADER_ENGINES_SHIFT?

(Joe can't type)

exactly right, thanks Michel

Applied with a compile fix.

Joe, in the future please make sure your patches compile before submitting them.

Signature · 6 lines
Earthling Michel Dänzer            |                  http://www.amd.com
Libre software enthusiast          |                Mesa and X developer
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
author
#9 lore raw

PATCH 3/11 aiptek: Fix probable mask then right shift defects

In reply to Joe Perches

Precedence of & and >> is not the same and is not left to right. shift has higher precedence and should be done after the mask.

Here the shifts are unnecessary and a non-zero value can be used as the test to set 1 or zero.

Signed-off-by Joe Perches <[email protected]>
---
 drivers/input/tablet/aiptek.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/input/tablet/aiptek.c +3 -3
index e7f966d..dee2bb9 100644
@@ -489,9 +489,9 @@ static void aiptek_irq(struct urb *urb)
 			 */
 			jitterable = data[1] & 0x07;
 
-			left = (data[1] & aiptek->curSetting.mouseButtonLeft >> 2) != 0 ? 1 : 0;
-			right = (data[1] & aiptek->curSetting.mouseButtonRight >> 2) != 0 ? 1 : 0;
-			middle = (data[1] & aiptek->curSetting.mouseButtonMiddle >> 2) != 0 ? 1 : 0;
+			left = data[1] & aiptek->curSetting.mouseButtonLeft ? 1 : 0;
+			right = data[1] & aiptek->curSetting.mouseButtonRight ? 1 : 0;
+			middle = data[1] & aiptek->curSetting.mouseButtonMiddle ? 1 : 0;
 
 			input_report_key(inputdev, BTN_LEFT, left);
 			input_report_key(inputdev, BTN_MIDDLE, middle);
Signature · 1 line
2.1.2
maintainer
#10 lore raw

Re: [PATCH 03/11] aiptek: Fix probable mask then right shift defects

Hi Joe,

Quoted 2 lines · Joe Perches

Precedence of & and >> is not the same and is not left to right. shift has higher precedence and should be done after the mask.

Looking at the protocol description the current code is exactly right. We want to "move" button bits first as in packet type 1 they are in a different place than in other packets.

I'll take a patch that adds parenthesis around shifts to make clear it is intended.

Thanks.

Quoted 29 lines

Here the shifts are unnecessary and a non-zero value can be used as the test to set 1 or zero.

Signed-off-by Joe Perches <[email protected]>
---
 drivers/input/tablet/aiptek.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/input/tablet/aiptek.c +3 -3
index e7f966d..dee2bb9 100644
@@ -489,9 +489,9 @@ static void aiptek_irq(struct urb *urb)
 			 */
 			jitterable = data[1] & 0x07;
 
-			left = (data[1] & aiptek->curSetting.mouseButtonLeft >> 2) != 0 ? 1 : 0;
-			right = (data[1] & aiptek->curSetting.mouseButtonRight >> 2) != 0 ? 1 : 0;
-			middle = (data[1] & aiptek->curSetting.mouseButtonMiddle >> 2) != 0 ? 1 : 0;
+			left = data[1] & aiptek->curSetting.mouseButtonLeft ? 1 : 0;
+			right = data[1] & aiptek->curSetting.mouseButtonRight ? 1 : 0;
+			middle = data[1] & aiptek->curSetting.mouseButtonMiddle ? 1 : 0;
 
 			input_report_key(inputdev, BTN_LEFT, left);
 			input_report_key(inputdev, BTN_MIDDLE, middle);
Signature · 1 line
2.1.2
Signature · 1 line
Dmitry
author
#11 lore raw

Re: [PATCH 03/11] aiptek: Fix probable mask then right shift defects

Quoted 1 line · Dmitry Torokhov

Hi Joe,

Hello Dmitry.

Quoted 10 lines · Joe Perches
Quoted 2 lines

Precedence of & and >> is not the same and is not left to right. shift has higher precedence and should be done after the mask.

Looking at the protocol description the current code is exactly right. We want to "move" button bits first as in packet type 1 they are in a different place than in other packets.

I'll take a patch that adds parenthesis around shifts to make clear it is intended.

I think it's more sensible to do the shift first to a temporary then direct comparisons.

Perhaps something like this cleanup that also uses a temporary for aiptek->curSetting and
!!(foo & mask) instead of ((foo & mask) != 0) ? 1 : 0;

---
 drivers/input/tablet/aiptek.c | 149 ++++++++++++++++++++----------------------
 1 file changed, 72 insertions(+), 77 deletions(-)
drivers/input/tablet/aiptek.c +72 -77
index e7f966d..9c46618 100644
@@ -433,6 +433,7 @@ static const char *map_val_to_str(const struct aiptek_map *map, int val)
 static void aiptek_irq(struct urb *urb)
 {
 	struct aiptek *aiptek = urb->context;
+	struct aiptek_settings *settings = &aiptek->curSetting;
 	unsigned char *data = aiptek->data;
 	struct input_dev *inputdev = aiptek->inputdev;
 	struct usb_interface *intf = aiptek->intf;

@@ -472,26 +473,31 @@ static void aiptek_irq(struct urb *urb)
 	 * tool generated the event.
 	 */
 	if (data[0] == 1) {
-		if (aiptek->curSetting.coordinateMode ==
+		if (settings->coordinateMode ==
 		    AIPTEK_COORDINATE_ABSOLUTE_MODE) {
 			aiptek->diagnostic =
 			    AIPTEK_DIAGNOSTIC_SENDING_RELATIVE_IN_ABSOLUTE;
 		} else {
-			x = (signed char) data[2];
-			y = (signed char) data[3];
-
-			/* jitterable keeps track of whether any button has been pressed.
-			 * We're also using it to remap the physical mouse button mask
-			 * to pseudo-settings. (We don't specifically care about it's
-			 * value after moving/transposing mouse button bitmasks, except
+			/*
+			 * Shift buttons pressed to the curSettings location.
+			 * jitterable keeps track of whether any button has
+			 * been pressed.  We're also using it to remap the
+			 * physical mouse button mask to pseudo-settings.
+			 * (We don't specifically care about it's value after
+			 * moving/transposing mouse button bitmasks, except
 			 * that a non-zero value indicates that one or more
 			 * mouse button was pressed.)
 			 */
+			unsigned char buttons = data[1] << 2;
+
+			x = (signed char)data[2];
+			y = (signed char)data[3];
+
 			jitterable = data[1] & 0x07;
 
-			left = (data[1] & aiptek->curSetting.mouseButtonLeft >> 2) != 0 ? 1 : 0;
-			right = (data[1] & aiptek->curSetting.mouseButtonRight >> 2) != 0 ? 1 : 0;
-			middle = (data[1] & aiptek->curSetting.mouseButtonMiddle >> 2) != 0 ? 1 : 0;
+			left = !!(buttons & settings->mouseButtonLeft);
+			right = !!(buttons & settings->mouseButtonRight);
+			middle = !!(buttons & settings->mouseButtonMiddle);
 
 			input_report_key(inputdev, BTN_LEFT, left);
 			input_report_key(inputdev, BTN_MIDDLE, middle);

@@ -505,10 +511,10 @@ static void aiptek_irq(struct urb *urb)
 			/* Wheel support is in the form of a single-event
 			 * firing.
 			 */
-			if (aiptek->curSetting.wheel != AIPTEK_WHEEL_DISABLE) {
+			if (settings->wheel != AIPTEK_WHEEL_DISABLE) {
 				input_report_rel(inputdev, REL_WHEEL,
-						 aiptek->curSetting.wheel);
-				aiptek->curSetting.wheel = AIPTEK_WHEEL_DISABLE;
+						 settings->wheel);
+				settings->wheel = AIPTEK_WHEEL_DISABLE;
 			}
 			if (aiptek->lastMacro != -1) {
 			        input_report_key(inputdev,

@@ -522,26 +528,26 @@ static void aiptek_irq(struct urb *urb)
 	 * absolute coordinates.
 	 */
 	else if (data[0] == 2) {
-		if (aiptek->curSetting.coordinateMode == AIPTEK_COORDINATE_RELATIVE_MODE) {
+		if (settings->coordinateMode == AIPTEK_COORDINATE_RELATIVE_MODE) {
 			aiptek->diagnostic = AIPTEK_DIAGNOSTIC_SENDING_ABSOLUTE_IN_RELATIVE;
 		} else if (!AIPTEK_POINTER_ALLOW_STYLUS_MODE
-			    (aiptek->curSetting.pointerMode)) {
+			    (settings->pointerMode)) {
 				aiptek->diagnostic = AIPTEK_DIAGNOSTIC_TOOL_DISALLOWED;
 		} else {
 			x = get_unaligned_le16(data + 1);
 			y = get_unaligned_le16(data + 3);
 			z = get_unaligned_le16(data + 6);
 
-			dv = (data[5] & 0x01) != 0 ? 1 : 0;
-			p = (data[5] & 0x02) != 0 ? 1 : 0;
-			tip = (data[5] & 0x04) != 0 ? 1 : 0;
+			dv = !!(data[5] & 0x01);
+			p = !!(data[5] & 0x02);
+			tip = !!(data[5] & 0x04);
 
 			/* Use jitterable to re-arrange button masks
 			 */
 			jitterable = data[5] & 0x18;
 
-			bs = (data[5] & aiptek->curSetting.stylusButtonLower) != 0 ? 1 : 0;
-			pck = (data[5] & aiptek->curSetting.stylusButtonUpper) != 0 ? 1 : 0;
+			bs = !!(data[5] & settings->stylusButtonLower);
+			pck = !!(data[5] & settings->stylusButtonUpper);
 
 			/* dv indicates 'data valid' (e.g., the tablet is in sync
 			 * and has delivered a "correct" report) We will ignore

@@ -552,14 +558,14 @@ static void aiptek_irq(struct urb *urb)
 				 * tool key, and set the new one.
 				 */
 				if (aiptek->previousToolMode !=
-				    aiptek->curSetting.toolMode) {
+				    settings->toolMode) {
 				        input_report_key(inputdev,
 							 aiptek->previousToolMode, 0);
 					input_report_key(inputdev,
-							 aiptek->curSetting.toolMode,
+							 settings->toolMode,
 							 1);
 					aiptek->previousToolMode =
-					          aiptek->curSetting.toolMode;
+					          settings->toolMode;
 				}
 
 				if (p != 0) {

@@ -571,27 +577,25 @@ static void aiptek_irq(struct urb *urb)
 					input_report_key(inputdev, BTN_STYLUS, bs);
 					input_report_key(inputdev, BTN_STYLUS2, pck);
 
-					if (aiptek->curSetting.xTilt !=
-					    AIPTEK_TILT_DISABLE) {
+					if (settings->xTilt != AIPTEK_TILT_DISABLE) {
 						input_report_abs(inputdev,
 								 ABS_TILT_X,
-								 aiptek->curSetting.xTilt);
+								 settings->xTilt);
 					}
-					if (aiptek->curSetting.yTilt != AIPTEK_TILT_DISABLE) {
+					if (settings->yTilt != AIPTEK_TILT_DISABLE) {
 						input_report_abs(inputdev,
 								 ABS_TILT_Y,
-								 aiptek->curSetting.yTilt);
+								 settings->yTilt);
 					}
 
 					/* Wheel support is in the form of a single-event
 					 * firing.
 					 */
-					if (aiptek->curSetting.wheel !=
-					    AIPTEK_WHEEL_DISABLE) {
+					if (settings->wheel != AIPTEK_WHEEL_DISABLE) {
 						input_report_abs(inputdev,
 								 ABS_WHEEL,
-								 aiptek->curSetting.wheel);
-						aiptek->curSetting.wheel = AIPTEK_WHEEL_DISABLE;
+								 settings->wheel);
+						settings->wheel = AIPTEK_WHEEL_DISABLE;
 					}
 				}
 				input_report_abs(inputdev, ABS_MISC, p | AIPTEK_REPORT_TOOL_STYLUS);

@@ -607,10 +611,10 @@ static void aiptek_irq(struct urb *urb)
 	/* Report 3's come from the mouse in absolute mode.
 	 */
 	else if (data[0] == 3) {
-		if (aiptek->curSetting.coordinateMode == AIPTEK_COORDINATE_RELATIVE_MODE) {
+		if (settings->coordinateMode == AIPTEK_COORDINATE_RELATIVE_MODE) {
 			aiptek->diagnostic = AIPTEK_DIAGNOSTIC_SENDING_ABSOLUTE_IN_RELATIVE;
 		} else if (!AIPTEK_POINTER_ALLOW_MOUSE_MODE
-			(aiptek->curSetting.pointerMode)) {
+			(settings->pointerMode)) {
 			aiptek->diagnostic = AIPTEK_DIAGNOSTIC_TOOL_DISALLOWED;
 		} else {
 			x = get_unaligned_le16(data + 1);

@@ -618,25 +622,25 @@ static void aiptek_irq(struct urb *urb)
 
 			jitterable = data[5] & 0x1c;
 
-			dv = (data[5] & 0x01) != 0 ? 1 : 0;
-			p = (data[5] & 0x02) != 0 ? 1 : 0;
-			left = (data[5] & aiptek->curSetting.mouseButtonLeft) != 0 ? 1 : 0;
-			right = (data[5] & aiptek->curSetting.mouseButtonRight) != 0 ? 1 : 0;
-			middle = (data[5] & aiptek->curSetting.mouseButtonMiddle) != 0 ? 1 : 0;
+			dv = !!(data[5] & 0x01);
+			p = !!(data[5] & 0x02);
+			left = !!(data[5] & settings->mouseButtonLeft);
+			right = !!(data[5] & settings->mouseButtonRight);
+			middle = !!(data[5] & settings->mouseButtonMiddle);
 
 			if (dv != 0) {
 				/* If the selected tool changed, reset the old
 				 * tool key, and set the new one.
 				 */
 				if (aiptek->previousToolMode !=
-				    aiptek->curSetting.toolMode) {
+				    settings->toolMode) {
 				        input_report_key(inputdev,
 							 aiptek->previousToolMode, 0);
 					input_report_key(inputdev,
-							 aiptek->curSetting.toolMode,
+							 settings->toolMode,
 							 1);
 					aiptek->previousToolMode =
-					          aiptek->curSetting.toolMode;
+					          settings->toolMode;
 				}
 
 				if (p != 0) {

@@ -650,11 +654,11 @@ static void aiptek_irq(struct urb *urb)
 					/* Wheel support is in the form of a single-event
 					 * firing.
 					 */
-					if (aiptek->curSetting.wheel != AIPTEK_WHEEL_DISABLE) {
+					if (settings->wheel != AIPTEK_WHEEL_DISABLE) {
 						input_report_abs(inputdev,
 								 ABS_WHEEL,
-								 aiptek->curSetting.wheel);
-						aiptek->curSetting.wheel = AIPTEK_WHEEL_DISABLE;
+								 settings->wheel);
+						settings->wheel = AIPTEK_WHEEL_DISABLE;
 					}
 				}
 				input_report_abs(inputdev, ABS_MISC, p | AIPTEK_REPORT_TOOL_MOUSE);

@@ -672,11 +676,11 @@ static void aiptek_irq(struct urb *urb)
 	else if (data[0] == 4) {
 		jitterable = data[1] & 0x18;
 
-		dv = (data[1] & 0x01) != 0 ? 1 : 0;
-		p = (data[1] & 0x02) != 0 ? 1 : 0;
-		tip = (data[1] & 0x04) != 0 ? 1 : 0;
-		bs = (data[1] & aiptek->curSetting.stylusButtonLower) != 0 ? 1 : 0;
-		pck = (data[1] & aiptek->curSetting.stylusButtonUpper) != 0 ? 1 : 0;
+		dv = !!(data[1] & 0x01);
+		p = !!(data[1] & 0x02);
+		tip = !!(data[1] & 0x04);
+		bs = !!(data[1] & settings->stylusButtonLower);
+		pck = !!(data[1] & settings->stylusButtonUpper);
 
 		macro = dv && p && tip && !(data[3] & 1) ? (data[3] >> 1) : -1;
 		z = get_unaligned_le16(data + 4);

@@ -685,15 +689,12 @@ static void aiptek_irq(struct urb *urb)
 		        /* If the selected tool changed, reset the old
 			 * tool key, and set the new one.
 			 */
-		        if (aiptek->previousToolMode !=
-			    aiptek->curSetting.toolMode) {
+		        if (aiptek->previousToolMode != settings->toolMode) {
 			        input_report_key(inputdev,
 						 aiptek->previousToolMode, 0);
 				input_report_key(inputdev,
-						 aiptek->curSetting.toolMode,
-						 1);
-				aiptek->previousToolMode =
-				        aiptek->curSetting.toolMode;
+						 settings->toolMode, 1);
+				aiptek->previousToolMode = settings->toolMode;
 			}
 		}
 

@@ -715,24 +716,23 @@ static void aiptek_irq(struct urb *urb)
 	else if (data[0] == 5) {
 		jitterable = data[1] & 0x1c;
 
-		dv = (data[1] & 0x01) != 0 ? 1 : 0;
-		p = (data[1] & 0x02) != 0 ? 1 : 0;
-		left = (data[1]& aiptek->curSetting.mouseButtonLeft) != 0 ? 1 : 0;
-		right = (data[1] & aiptek->curSetting.mouseButtonRight) != 0 ? 1 : 0;
-		middle = (data[1] & aiptek->curSetting.mouseButtonMiddle) != 0 ? 1 : 0;
+		dv = !!(data[1] & 0x01);
+		p = !!(data[1] & 0x02);
+		left = !!(data[1]& settings->mouseButtonLeft);
+		right = !!(data[1] & settings->mouseButtonRight);
+		middle = !!(data[1] & settings->mouseButtonMiddle);
 		macro = dv && p && left && !(data[3] & 1) ? (data[3] >> 1) : 0;
 
 		if (dv) {
 		        /* If the selected tool changed, reset the old
 			 * tool key, and set the new one.
 			 */
-		        if (aiptek->previousToolMode !=
-			    aiptek->curSetting.toolMode) {
+		        if (aiptek->previousToolMode != settings->toolMode) {
 		                input_report_key(inputdev,
 						 aiptek->previousToolMode, 0);
 			        input_report_key(inputdev,
-						 aiptek->curSetting.toolMode, 1);
-			        aiptek->previousToolMode = aiptek->curSetting.toolMode;
+						 settings->toolMode, 1);
+			        aiptek->previousToolMode = settings->toolMode;
 			}
 		}
 

@@ -770,15 +770,10 @@ static void aiptek_irq(struct urb *urb)
 		/* If the selected tool changed, reset the old
 		   tool key, and set the new one.
 		*/
-		if (aiptek->previousToolMode !=
-		    aiptek->curSetting.toolMode) {
-		        input_report_key(inputdev,
-					 aiptek->previousToolMode, 0);
-			input_report_key(inputdev,
-					 aiptek->curSetting.toolMode,
-					 1);
-			aiptek->previousToolMode =
-				aiptek->curSetting.toolMode;
+		if (aiptek->previousToolMode != settings->toolMode) {
+		        input_report_key(inputdev, aiptek->previousToolMode, 0);
+			input_report_key(inputdev, settings->toolMode, 1);
+			aiptek->previousToolMode = settings->toolMode;
 		}
 
 		input_report_key(inputdev, macroKeyEvents[macro], 1);

@@ -802,9 +797,9 @@ static void aiptek_irq(struct urb *urb)
 	 */
 
 	if (aiptek->previousJitterable != jitterable &&
-	    aiptek->curSetting.jitterDelay != 0 && aiptek->inDelay != 1) {
+	    settings->jitterDelay != 0 && aiptek->inDelay != 1) {
 		aiptek->endDelay = jiffies +
-		    ((aiptek->curSetting.jitterDelay * HZ) / 1000);
+			((settings->jitterDelay * HZ) / 1000);
 		aiptek->inDelay = 1;
 	}
 	aiptek->previousJitterable = jitterable;
maintainer
#12 lore raw

Re: [PATCH 03/11] aiptek: Fix probable mask then right shift defects

Quoted 16 lines · Joe Perches
Quoted 1 line · Dmitry Torokhov

Hi Joe,

Hello Dmitry.

Quoted 10 lines · Joe Perches
Quoted 2 lines

Precedence of & and >> is not the same and is not left to right. shift has higher precedence and should be done after the mask.

Looking at the protocol description the current code is exactly right. We want to "move" button bits first as in packet type 1 they are in a different place than in other packets.

I'll take a patch that adds parenthesis around shifts to make clear it is intended.

Quoted 6 lines

I think it's more sensible to do the shift first to a temporary then direct comparisons.

Perhaps something like this cleanup that also uses a temporary for aiptek->curSetting and
!!(foo & mask) instead of ((foo & mask) != 0) ? 1 : 0;

Unless you have the device I'd rather kept the changes (which are mostly cosmetic in nature and do not fix any bugs) to a minimum.

Thanks.

Signature · 1 line
Dmitry
author
#13 lore raw

Re: [PATCH 03/11] aiptek: Fix probable mask then right shift defects

Quoted 10 lines · Dmitry Torokhov
Quoted 9 lines · Joe Perches
Quoted 8 lines · Dmitry Torokhov
Quoted 2 lines · Joe Perches

Precedence of & and >> is not the same and is not left to right. shift has higher precedence and should be done after the mask.

Looking at the protocol description the current code is exactly right. We want to "move" button bits first as in packet type 1 they are in a different place than in other packets.

Quoted 5 lines
Quoted 5 lines
Quoted 2 lines

I'll take a patch that adds parenthesis around shifts to make clear it is intended.

I think it's more sensible to do the shift first to a temporary then direct comparisons.

[]

Quoted 2 lines

Unless you have the device I'd rather kept the changes (which are mostly cosmetic in nature and do not fix any bugs) to a minimum.

I don't have the device.
I think you should do what you think appropriate.

cheers, Joe

author
#14 lore raw

PATCH 4/11 dvb-net: Fix probable mask then right shift defects

In reply to Joe Perches

Precedence of & and >> is not the same and is not left to right. shift has higher precedence and should be done after the mask.

Add parentheses around the mask.

Signed-off-by Joe Perches <[email protected]>
---
 drivers/media/dvb-core/dvb_net.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/media/dvb-core/dvb_net.c +3 -1
index 059e611..441814b 100644
@@ -379,7 +379,9 @@ static void dvb_net_ule( struct net_device *dev, const u8 *buf, size_t buf_len )
 			/* Check TS error conditions: sync_byte, transport_error_indicator, scrambling_control . */
 			if ((ts[0] != TS_SYNC) || (ts[1] & TS_TEI) || ((ts[3] & TS_SC) != 0)) {
 				printk(KERN_WARNING "%lu: Invalid TS cell: SYNC %#x, TEI %u, SC %#x.\n",
-				       priv->ts_count, ts[0], ts[1] & TS_TEI >> 7, ts[3] & 0xC0 >> 6);
+				       priv->ts_count, ts[0],
+				       (ts[1] & TS_TEI) >> 7,
+				       (ts[3] & 0xC0) >> 6);
 
 				/* Drop partly decoded SNDU, reset state, resync on PUSI. */
 				if (priv->ule_skb) {
Signature · 1 line
2.1.2
author
#15 lore raw

PATCH 5/11 cx25840/cx18: Use standard ordering of mask and shift

In reply to Joe Perches

Precedence of & and >> is not the same and is not left to right. shift has higher precedence and should be done after the mask.

This use has a mask then shift which is not the normal style.

Move the shift before the mask to match nearly all the other uses in kernel.

Signed-off-by Joe Perches <[email protected]>
---
 drivers/media/i2c/cx25840/cx25840-core.c | 12 ++++++------
 drivers/media/pci/cx18/cx18-av-core.c    | 16 ++++++++--------
 2 files changed, 14 insertions(+), 14 deletions(-)
drivers/media/i2c/cx25840/cx25840-core.c +6 -6
index e453a3f..0327032 100644
@@ -879,7 +879,7 @@ void cx25840_std_setup(struct i2c_client *client)
 	/* Sets horizontal blanking delay and active lines */
 	cx25840_write(client, 0x470, hblank);
 	cx25840_write(client, 0x471,
-			0xff & (((hblank >> 8) & 0x3) | (hactive << 4)));
+		      (((hblank >> 8) & 0x3) | (hactive << 4)) & 0xff);
 	cx25840_write(client, 0x472, hactive >> 4);
 
 	/* Sets burst gate delay */

@@ -888,13 +888,13 @@ void cx25840_std_setup(struct i2c_client *client)
 	/* Sets vertical blanking delay and active duration */
 	cx25840_write(client, 0x474, vblank);
 	cx25840_write(client, 0x475,
-			0xff & (((vblank >> 8) & 0x3) | (vactive << 4)));
+		      (((vblank >> 8) & 0x3) | (vactive << 4)) & 0xff);
 	cx25840_write(client, 0x476, vactive >> 4);
 	cx25840_write(client, 0x477, vblank656);
 
 	/* Sets src decimation rate */
-	cx25840_write(client, 0x478, 0xff & src_decimation);
-	cx25840_write(client, 0x479, 0xff & (src_decimation >> 8));
+	cx25840_write(client, 0x478, src_decimation & 0xff);
+	cx25840_write(client, 0x479, (src_decimation >> 8) & 0xff);
 
 	/* Sets Luma and UV Low pass filters */
 	cx25840_write(client, 0x47a, luma_lpf << 6 | ((uv_lpf << 4) & 0x30));

@@ -904,8 +904,8 @@ void cx25840_std_setup(struct i2c_client *client)
 
 	/* Sets SC Step*/
 	cx25840_write(client, 0x47c, sc);
-	cx25840_write(client, 0x47d, 0xff & sc >> 8);
-	cx25840_write(client, 0x47e, 0xff & sc >> 16);
+	cx25840_write(client, 0x47d, (sc >> 8) & 0xff);
+	cx25840_write(client, 0x47e, (sc >> 16) & 0xff);
 
 	/* Sets VBI parameters */
 	if (std & V4L2_STD_625_50) {
drivers/media/pci/cx18/cx18-av-core.c +8 -8
index 2d3afe0..45be26c 100644
@@ -490,8 +490,8 @@ void cx18_av_std_setup(struct cx18 *cx)
 
 	/* Sets horizontal blanking delay and active lines */
 	cx18_av_write(cx, 0x470, hblank);
-	cx18_av_write(cx, 0x471, 0xff & (((hblank >> 8) & 0x3) |
-						(hactive << 4)));
+	cx18_av_write(cx, 0x471,
+		      (((hblank >> 8) & 0x3) | (hactive << 4)) & 0xff);
 	cx18_av_write(cx, 0x472, hactive >> 4);
 
 	/* Sets burst gate delay */

@@ -499,14 +499,14 @@ void cx18_av_std_setup(struct cx18 *cx)
 
 	/* Sets vertical blanking delay and active duration */
 	cx18_av_write(cx, 0x474, vblank);
-	cx18_av_write(cx, 0x475, 0xff & (((vblank >> 8) & 0x3) |
-						(vactive << 4)));
+	cx18_av_write(cx, 0x475,
+		      (((vblank >> 8) & 0x3) | (vactive << 4)) & 0xff);
 	cx18_av_write(cx, 0x476, vactive >> 4);
 	cx18_av_write(cx, 0x477, vblank656);
 
 	/* Sets src decimation rate */
-	cx18_av_write(cx, 0x478, 0xff & src_decimation);
-	cx18_av_write(cx, 0x479, 0xff & (src_decimation >> 8));
+	cx18_av_write(cx, 0x478, src_decimation & 0xff);
+	cx18_av_write(cx, 0x479, (src_decimation >> 8) & 0xff);
 
 	/* Sets Luma and UV Low pass filters */
 	cx18_av_write(cx, 0x47a, luma_lpf << 6 | ((uv_lpf << 4) & 0x30));

@@ -516,8 +516,8 @@ void cx18_av_std_setup(struct cx18 *cx)
 
 	/* Sets SC Step*/
 	cx18_av_write(cx, 0x47c, sc);
-	cx18_av_write(cx, 0x47d, 0xff & sc >> 8);
-	cx18_av_write(cx, 0x47e, 0xff & sc >> 16);
+	cx18_av_write(cx, 0x47d, (sc >> 8) & 0xff);
+	cx18_av_write(cx, 0x47e, (sc >> 16) & 0xff);
 
 	if (std & V4L2_STD_625_50) {
 		state->slicer_line_delay = 1;
Signature · 1 line
2.1.2
maintainer Reviewed-by on v1 5/11
#16 lore raw

Re: [PATCH 05/11] cx25840/cx18: Use standard ordering of mask and shift

Quoted 9 lines · Joe Perches

Precedence of & and >> is not the same and is not left to right. shift has higher precedence and should be done after the mask.

This use has a mask then shift which is not the normal style.

Move the shift before the mask to match nearly all the other uses in kernel.

Signed-off-by Joe Perches <[email protected]>

The patch is technically correct.

Reviewed-by Andy Walls <[email protected]>
Quoted 91 lines
---
 drivers/media/i2c/cx25840/cx25840-core.c | 12 ++++++------
 drivers/media/pci/cx18/cx18-av-core.c    | 16 ++++++++--------
 2 files changed, 14 insertions(+), 14 deletions(-)
drivers/media/i2c/cx25840/cx25840-core.c +6 -6
index e453a3f..0327032 100644
@@ -879,7 +879,7 @@ void cx25840_std_setup(struct i2c_client *client)
 	/* Sets horizontal blanking delay and active lines */
 	cx25840_write(client, 0x470, hblank);
 	cx25840_write(client, 0x471,
-			0xff & (((hblank >> 8) & 0x3) | (hactive << 4)));
+		      (((hblank >> 8) & 0x3) | (hactive << 4)) & 0xff);
 	cx25840_write(client, 0x472, hactive >> 4);
 
 	/* Sets burst gate delay */

@@ -888,13 +888,13 @@ void cx25840_std_setup(struct i2c_client *client)
 	/* Sets vertical blanking delay and active duration */
 	cx25840_write(client, 0x474, vblank);
 	cx25840_write(client, 0x475,
-			0xff & (((vblank >> 8) & 0x3) | (vactive << 4)));
+		      (((vblank >> 8) & 0x3) | (vactive << 4)) & 0xff);
 	cx25840_write(client, 0x476, vactive >> 4);
 	cx25840_write(client, 0x477, vblank656);
 
 	/* Sets src decimation rate */
-	cx25840_write(client, 0x478, 0xff & src_decimation);
-	cx25840_write(client, 0x479, 0xff & (src_decimation >> 8));
+	cx25840_write(client, 0x478, src_decimation & 0xff);
+	cx25840_write(client, 0x479, (src_decimation >> 8) & 0xff);
 
 	/* Sets Luma and UV Low pass filters */
 	cx25840_write(client, 0x47a, luma_lpf << 6 | ((uv_lpf << 4) & 0x30));

@@ -904,8 +904,8 @@ void cx25840_std_setup(struct i2c_client *client)
 
 	/* Sets SC Step*/
 	cx25840_write(client, 0x47c, sc);
-	cx25840_write(client, 0x47d, 0xff & sc >> 8);
-	cx25840_write(client, 0x47e, 0xff & sc >> 16);
+	cx25840_write(client, 0x47d, (sc >> 8) & 0xff);
+	cx25840_write(client, 0x47e, (sc >> 16) & 0xff);
 
 	/* Sets VBI parameters */
 	if (std & V4L2_STD_625_50) {
drivers/media/pci/cx18/cx18-av-core.c +8 -8
index 2d3afe0..45be26c 100644
@@ -490,8 +490,8 @@ void cx18_av_std_setup(struct cx18 *cx)
 
 	/* Sets horizontal blanking delay and active lines */
 	cx18_av_write(cx, 0x470, hblank);
-	cx18_av_write(cx, 0x471, 0xff & (((hblank >> 8) & 0x3) |
-						(hactive << 4)));
+	cx18_av_write(cx, 0x471,
+		      (((hblank >> 8) & 0x3) | (hactive << 4)) & 0xff);
 	cx18_av_write(cx, 0x472, hactive >> 4);
 
 	/* Sets burst gate delay */

@@ -499,14 +499,14 @@ void cx18_av_std_setup(struct cx18 *cx)
 
 	/* Sets vertical blanking delay and active duration */
 	cx18_av_write(cx, 0x474, vblank);
-	cx18_av_write(cx, 0x475, 0xff & (((vblank >> 8) & 0x3) |
-						(vactive << 4)));
+	cx18_av_write(cx, 0x475,
+		      (((vblank >> 8) & 0x3) | (vactive << 4)) & 0xff);
 	cx18_av_write(cx, 0x476, vactive >> 4);
 	cx18_av_write(cx, 0x477, vblank656);
 
 	/* Sets src decimation rate */
-	cx18_av_write(cx, 0x478, 0xff & src_decimation);
-	cx18_av_write(cx, 0x479, 0xff & (src_decimation >> 8));
+	cx18_av_write(cx, 0x478, src_decimation & 0xff);
+	cx18_av_write(cx, 0x479, (src_decimation >> 8) & 0xff);
 
 	/* Sets Luma and UV Low pass filters */
 	cx18_av_write(cx, 0x47a, luma_lpf << 6 | ((uv_lpf << 4) & 0x30));

@@ -516,8 +516,8 @@ void cx18_av_std_setup(struct cx18 *cx)
 
 	/* Sets SC Step*/
 	cx18_av_write(cx, 0x47c, sc);
-	cx18_av_write(cx, 0x47d, 0xff & sc >> 8);
-	cx18_av_write(cx, 0x47e, 0xff & sc >> 16);
+	cx18_av_write(cx, 0x47d, (sc >> 8) & 0xff);
+	cx18_av_write(cx, 0x47e, (sc >> 16) & 0xff);
 
 	if (std & V4L2_STD_625_50) {
 		state->slicer_line_delay = 1;
author
#17 lore raw

PATCH 6/11 wm8350-core: Fix probable mask then right shift defect

In reply to Joe Perches

Precedence of & and >> is not the same and is not left to right. shift has higher precedence and should be done after the mask.

Add parentheses around the mask.

Signed-off-by Joe Perches <[email protected]>
---
 drivers/mfd/wm8350-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/mfd/wm8350-core.c +1 -1
index 4ab527f..f5124a8 100644
@@ -308,7 +308,7 @@ int wm8350_device_init(struct wm8350 *wm8350, int irq,
 		goto err;
 	}
 
-	mode = id2 & WM8350_CONF_STS_MASK >> 10;
+	mode = (id2 & WM8350_CONF_STS_MASK) >> 10;
 	cust_id = id2 & WM8350_CUST_ID_MASK;
 	chip_rev = (id2 & WM8350_CHIP_REV_MASK) >> 12;
 	dev_info(wm8350->dev,
Signature · 1 line
2.1.2
#19 lore raw

Re: [PATCH 06/11] wm8350-core: Fix probable mask then right shift defect

In reply to Joe Perches
Quoted 9 lines · Joe Perches

Precedence of & and >> is not the same and is not left to right. shift has higher precedence and should be done after the mask.

Add parentheses around the mask.

Signed-off-by Joe Perches <[email protected]>
---
 drivers/mfd/wm8350-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Applied with Charles' Ack.

Quoted 13 lines
drivers/mfd/wm8350-core.c +1 -1
index 4ab527f..f5124a8 100644
@@ -308,7 +308,7 @@ int wm8350_device_init(struct wm8350 *wm8350, int irq,
 		goto err;
 	}
 
-	mode = id2 & WM8350_CONF_STS_MASK >> 10;
+	mode = (id2 & WM8350_CONF_STS_MASK) >> 10;
 	cust_id = id2 & WM8350_CUST_ID_MASK;
 	chip_rev = (id2 & WM8350_CHIP_REV_MASK) >> 12;
 	dev_info(wm8350->dev,
Signature · 4 lines
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
author
#20 lore raw

PATCH 7/11 iwlwifi: dvm: Fix probable mask then right shift defect

In reply to Joe Perches

Precedence of & and >> is not the same and is not left to right. shift has higher precedence and should be done after the mask.

Add parentheses around the mask.

Signed-off-by Joe Perches <[email protected]>
---
 drivers/net/wireless/iwlwifi/dvm/lib.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/net/wireless/iwlwifi/dvm/lib.c +2 -2
index 2191621..02e4ede 100644
@@ -418,8 +418,8 @@ void iwlagn_bt_adjust_rssi_monitor(struct iwl_priv *priv, bool rssi_ena)
 
 static bool iwlagn_bt_traffic_is_sco(struct iwl_bt_uart_msg *uart_msg)
 {
-	return BT_UART_MSG_FRAME3SCOESCO_MSK & uart_msg->frame3 >>
-			BT_UART_MSG_FRAME3SCOESCO_POS;
+	return (BT_UART_MSG_FRAME3SCOESCO_MSK & uart_msg->frame3) >>
+		BT_UART_MSG_FRAME3SCOESCO_POS;
 }
 
 static void iwlagn_bt_traffic_change_work(struct work_struct *work)
Signature · 1 line
2.1.2
#21 lore raw

RE: [PATCH 07/11] iwlwifi: dvm: Fix probable mask then right shift defect

Quoted 8 lines

Precedence of & and >> is not the same and is not left to right. shift has higher precedence and should be done after the mask.

Add parentheses around the mask.

Signed-off-by Joe Perches <[email protected]>

---

Applied - thanks.

author
#22 lore raw

PATCH 8/11 ssb: driver_chip_comon_pmu: Fix probable mask then right shift defect

In reply to Joe Perches

Precedence of & and >> is not the same and is not left to right. shift has higher precedence and should be done after the mask.

Add parentheses around the mask.

Signed-off-by Joe Perches <[email protected]>
---
 drivers/ssb/driver_chipcommon_pmu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/ssb/driver_chipcommon_pmu.c +2 -2
index 1173a09..bc71583 100644
@@ -621,8 +621,8 @@ static u32 ssb_pmu_get_alp_clock_clk0(struct ssb_chipcommon *cc)
 	u32 crystalfreq;
 	const struct pmu0_plltab_entry *e = NULL;
 
-	crystalfreq = chipco_read32(cc, SSB_CHIPCO_PMU_CTL) &
-		      SSB_CHIPCO_PMU_CTL_XTALFREQ >> SSB_CHIPCO_PMU_CTL_XTALFREQ_SHIFT;
+	crystalfreq = (chipco_read32(cc, SSB_CHIPCO_PMU_CTL) &
+		       SSB_CHIPCO_PMU_CTL_XTALFREQ) >> SSB_CHIPCO_PMU_CTL_XTALFREQ_SHIFT;
 	e = pmu0_plltab_find_entry(crystalfreq);
 	BUG_ON(!e);
 	return e->freq * 1000;
Signature · 1 line
2.1.2
maintainer Reviewed-by on v1 8/11
#23 lore raw

Re: [PATCH 08/11] ssb: driver_chip_comon_pmu: Fix probable mask then right shift defect

Quoted 6 lines · On Sun, 26 Oct 2014 22:25:04 -0700 Joe Perches <[email protected]> wrote:

Precedence of & and >> is not the same and is not left to right. shift has higher precedence and should be done after the mask.

Add parentheses around the mask.

Signed-off-by Joe Perches <[email protected]>

Good catch.

Reviewed-by Michael Büsch <[email protected]>
Quoted 19 lines
---
 drivers/ssb/driver_chipcommon_pmu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/ssb/driver_chipcommon_pmu.c +2 -2
index 1173a09..bc71583 100644
@@ -621,8 +621,8 @@ static u32 ssb_pmu_get_alp_clock_clk0(struct ssb_chipcommon *cc)
 	u32 crystalfreq;
 	const struct pmu0_plltab_entry *e = NULL;
 
-	crystalfreq = chipco_read32(cc, SSB_CHIPCO_PMU_CTL) &
-		      SSB_CHIPCO_PMU_CTL_XTALFREQ >> SSB_CHIPCO_PMU_CTL_XTALFREQ_SHIFT;
+	crystalfreq = (chipco_read32(cc, SSB_CHIPCO_PMU_CTL) &
+		       SSB_CHIPCO_PMU_CTL_XTALFREQ) >> SSB_CHIPCO_PMU_CTL_XTALFREQ_SHIFT;
 	e = pmu0_plltab_find_entry(crystalfreq);
 	BUG_ON(!e);
 	return e->freq * 1000;
Signature · 1 line
Michael
author
#24 lore raw

PATCH 9/11 tty: ipwireless: Fix probable mask then right shift defects

In reply to Joe Perches

Precedence of & and >> is not the same and is not left to right. shift has higher precedence and should be done after the mask.

Add parentheses around the masks.

Signed-off-by Joe Perches <[email protected]>
---
 drivers/tty/ipwireless/hardware.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
drivers/tty/ipwireless/hardware.c +6 -6
index 2c14842..5c77e1e 100644
@@ -378,9 +378,9 @@ static void swap_packet_bitfield_to_le(unsigned char *data)
 	/*
 	 * transform bits from aa.bbb.ccc to ccc.bbb.aa
 	 */
-	ret |= tmp & 0xc0 >> 6;
-	ret |= tmp & 0x38 >> 1;
-	ret |= tmp & 0x07 << 5;
+	ret |= (tmp & 0xc0) >> 6;
+	ret |= (tmp & 0x38) >> 1;
+	ret |= (tmp & 0x07) << 5;
 	*data = ret & 0xff;
 #endif
 }

@@ -393,9 +393,9 @@ static void swap_packet_bitfield_from_le(unsigned char *data)
 	/*
 	 * transform bits from ccc.bbb.aa to aa.bbb.ccc
 	 */
-	ret |= tmp & 0xe0 >> 5;
-	ret |= tmp & 0x1c << 1;
-	ret |= tmp & 0x03 << 6;
+	ret |= (tmp & 0xe0) >> 5;
+	ret |= (tmp & 0x1c) << 1;
+	ret |= (tmp & 0x03) << 6;
 	*data = ret & 0xff;
 #endif
 }
Signature · 1 line
2.1.2
Reviewed-by on v1 9/11
#26 lore raw

Re: [PATCH 09/11] tty: ipwireless: Fix probable mask then right shift defects

In reply to Joe Perches
Quoted 6 lines · Joe Perches

Precedence of & and >> is not the same and is not left to right. shift has higher precedence and should be done after the mask.

Add parentheses around the masks.

Signed-off-by Joe Perches <[email protected]>
Reviewed-by Jiri Kosina <[email protected]>

Greg, can you take this through your char/misc tree, please?

Thanks.

Quoted 37 lines
---
 drivers/tty/ipwireless/hardware.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
drivers/tty/ipwireless/hardware.c +6 -6
index 2c14842..5c77e1e 100644
@@ -378,9 +378,9 @@ static void swap_packet_bitfield_to_le(unsigned char *data)
 	/*
 	 * transform bits from aa.bbb.ccc to ccc.bbb.aa
 	 */
-	ret |= tmp & 0xc0 >> 6;
-	ret |= tmp & 0x38 >> 1;
-	ret |= tmp & 0x07 << 5;
+	ret |= (tmp & 0xc0) >> 6;
+	ret |= (tmp & 0x38) >> 1;
+	ret |= (tmp & 0x07) << 5;
 	*data = ret & 0xff;
 #endif
 }

@@ -393,9 +393,9 @@ static void swap_packet_bitfield_from_le(unsigned char *data)
 	/*
 	 * transform bits from ccc.bbb.aa to aa.bbb.ccc
 	 */
-	ret |= tmp & 0xe0 >> 5;
-	ret |= tmp & 0x1c << 1;
-	ret |= tmp & 0x03 << 6;
+	ret |= (tmp & 0xe0) >> 5;
+	ret |= (tmp & 0x1c) << 1;
+	ret |= (tmp & 0x03) << 6;
 	*data = ret & 0xff;
 #endif
 }
Signature · 1 line
2.1.2
Signature · 2 lines
Jiri Kosina
SUSE Labs
maintainer
#27 lore raw

Re: [PATCH 09/11] tty: ipwireless: Fix probable mask then right shift defects

Quoted 5 lines · Jiri Kosina
Quoted 3 lines · Joe Perches

Precedence of & and >> is not the same and is not left to right. shift has higher precedence and should be done after the mask.

Quoted 7 lines
Quoted 3 lines

Add parentheses around the masks.

Signed-off-by Joe Perches <[email protected]>
Reviewed-by Jiri Kosina <[email protected]>

Greg, can you take this through your char/misc tree, please?

Ok, will do, give me a week or so to catch up on pending patches.

greg k-h

author
#28 lore raw

PATCH 10/11 hwa-hc: Fix probable mask then right shift defect

In reply to Joe Perches

Precedence of & and >> is not the same and is not left to right. shift has higher precedence and should be done after the mask.

Add parentheses around the mask.

Signed-off-by Joe Perches <[email protected]>
---
 drivers/usb/host/hwa-hc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/usb/host/hwa-hc.c +1 -1
index d0d8fad..1db0626 100644
@@ -607,7 +607,7 @@ found:
 	wa->wa_descr = wa_descr = (struct usb_wa_descriptor *) hdr;
 	if (le16_to_cpu(wa_descr->bcdWAVersion) > 0x0100)
 		dev_warn(dev, "Wire Adapter v%d.%d newer than groked v1.0\n",
-			 le16_to_cpu(wa_descr->bcdWAVersion) & 0xff00 >> 8,
+			 (le16_to_cpu(wa_descr->bcdWAVersion) & 0xff00) >> 8,
 			 le16_to_cpu(wa_descr->bcdWAVersion) & 0x00ff);
 	result = 0;
 error:
Signature · 1 line
2.1.2
author
#29 lore raw

PATCH 11/11 sound: ad1889: Fix probable mask then right shift defects

In reply to Joe Perches

Precedence of & and >> is not the same and is not left to right. shift has higher precedence and should be done after the mask.

Add parentheses around the mask.

Signed-off-by Joe Perches <[email protected]>
---
 sound/pci/ad1889.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
sound/pci/ad1889.c +4 -4
index 7bfdf9c..1610c38 100644
@@ -681,7 +681,7 @@ snd_ad1889_proc_read(struct snd_info_entry *entry, struct snd_info_buffer *buffe
 	
 	/* WARQ is at offset 12 */
 	tmp = (reg & AD_DS_WSMC_WARQ) ?
-			(((reg & AD_DS_WSMC_WARQ >> 12) & 0x01) ? 12 : 18) : 4;
+		((((reg & AD_DS_WSMC_WARQ) >> 12) & 0x01) ? 12 : 18) : 4;
 	tmp /= (reg & AD_DS_WSMC_WAST) ? 2 : 1;
 	
 	snd_iprintf(buffer, "Wave FIFO: %d %s words\n\n", tmp,

@@ -693,7 +693,7 @@ snd_ad1889_proc_read(struct snd_info_entry *entry, struct snd_info_buffer *buffe
 	
 	/* SYRQ is at offset 4 */
 	tmp = (reg & AD_DS_WSMC_SYRQ) ?
-			(((reg & AD_DS_WSMC_SYRQ >> 4) & 0x01) ? 12 : 18) : 4;
+		((((reg & AD_DS_WSMC_SYRQ) >> 4) & 0x01) ? 12 : 18) : 4;
 	tmp /= (reg & AD_DS_WSMC_WAST) ? 2 : 1;
 	
 	snd_iprintf(buffer, "Synthesis FIFO: %d %s words\n\n", tmp,

@@ -709,7 +709,7 @@ snd_ad1889_proc_read(struct snd_info_entry *entry, struct snd_info_buffer *buffe
 	
 	/* ACRQ is at offset 4 */
 	tmp = (reg & AD_DS_RAMC_ACRQ) ?
-			(((reg & AD_DS_RAMC_ACRQ >> 4) & 0x01) ? 12 : 18) : 4;
+		((((reg & AD_DS_RAMC_ACRQ) >> 4) & 0x01) ? 12 : 18) : 4;
 	tmp /= (reg & AD_DS_RAMC_ADST) ? 2 : 1;
 	
 	snd_iprintf(buffer, "ADC FIFO: %d %s words\n\n", tmp,

@@ -720,7 +720,7 @@ snd_ad1889_proc_read(struct snd_info_entry *entry, struct snd_info_buffer *buffe
 			
 	/* RERQ is at offset 12 */
 	tmp = (reg & AD_DS_RAMC_RERQ) ?
-			(((reg & AD_DS_RAMC_RERQ >> 12) & 0x01) ? 12 : 18) : 4;
+		((((reg & AD_DS_RAMC_RERQ) >> 12) & 0x01) ? 12 : 18) : 4;
 	tmp /= (reg & AD_DS_RAMC_ADST) ? 2 : 1;
 	
 	snd_iprintf(buffer, "Resampler FIFO: %d %s words\n\n", tmp,
Signature · 1 line
2.1.2
Applied
#30 lore raw

Re: [PATCH 11/11] sound: ad1889: Fix probable mask then right shift defects

At Sun, 26 Oct 2014 22:25:07 -0700,

Quoted 7 lines · Joe Perches

Precedence of & and >> is not the same and is not left to right. shift has higher precedence and should be done after the mask.

Add parentheses around the mask.

Signed-off-by Joe Perches <[email protected]>

Thanks, applied.

Takashi

Quoted 47 lines
---
 sound/pci/ad1889.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
sound/pci/ad1889.c +4 -4
index 7bfdf9c..1610c38 100644
@@ -681,7 +681,7 @@ snd_ad1889_proc_read(struct snd_info_entry *entry, struct snd_info_buffer *buffe
 
 	/* WARQ is at offset 12 */
 	tmp = (reg & AD_DS_WSMC_WARQ) ?
-			(((reg & AD_DS_WSMC_WARQ >> 12) & 0x01) ? 12 : 18) : 4;
+		((((reg & AD_DS_WSMC_WARQ) >> 12) & 0x01) ? 12 : 18) : 4;
 	tmp /= (reg & AD_DS_WSMC_WAST) ? 2 : 1;
 
 	snd_iprintf(buffer, "Wave FIFO: %d %s words\n\n", tmp,

@@ -693,7 +693,7 @@ snd_ad1889_proc_read(struct snd_info_entry *entry, struct snd_info_buffer *buffe
 
 	/* SYRQ is at offset 4 */
 	tmp = (reg & AD_DS_WSMC_SYRQ) ?
-			(((reg & AD_DS_WSMC_SYRQ >> 4) & 0x01) ? 12 : 18) : 4;
+		((((reg & AD_DS_WSMC_SYRQ) >> 4) & 0x01) ? 12 : 18) : 4;
 	tmp /= (reg & AD_DS_WSMC_WAST) ? 2 : 1;
 
 	snd_iprintf(buffer, "Synthesis FIFO: %d %s words\n\n", tmp,

@@ -709,7 +709,7 @@ snd_ad1889_proc_read(struct snd_info_entry *entry, struct snd_info_buffer *buffe
 
 	/* ACRQ is at offset 4 */
 	tmp = (reg & AD_DS_RAMC_ACRQ) ?
-			(((reg & AD_DS_RAMC_ACRQ >> 4) & 0x01) ? 12 : 18) : 4;
+		((((reg & AD_DS_RAMC_ACRQ) >> 4) & 0x01) ? 12 : 18) : 4;
 	tmp /= (reg & AD_DS_RAMC_ADST) ? 2 : 1;
 
 	snd_iprintf(buffer, "ADC FIFO: %d %s words\n\n", tmp,

@@ -720,7 +720,7 @@ snd_ad1889_proc_read(struct snd_info_entry *entry, struct snd_info_buffer *buffe
 
 	/* RERQ is at offset 12 */
 	tmp = (reg & AD_DS_RAMC_RERQ) ?
-			(((reg & AD_DS_RAMC_RERQ >> 12) & 0x01) ? 12 : 18) : 4;
+		((((reg & AD_DS_RAMC_RERQ) >> 12) & 0x01) ? 12 : 18) : 4;
 	tmp /= (reg & AD_DS_RAMC_ADST) ? 2 : 1;
 
 	snd_iprintf(buffer, "Resampler FIFO: %d %s words\n\n", tmp,
Signature · 1 line
2.1.2